GNU bug report logs - #14070
24.3.50; incorrect doc from `C-h f' when use `defadvice' with `before'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14070 in the body.
You can then email your comments to 14070 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: <bug-gnu-emacs <at> gnu.org>
Subject: 24.3.50; incorrect doc from `C-h f' when use `defadvice' with `before'
Date: Wed, 27 Mar 2013 10:24:10 -0700

emacs -Q
 
Evaluate this sexp:
 
(defadvice query-replace
 (before respect-search/replace-region-as-default-flag activate)
  nil
  (interactive
   (let ((common
          (query-replace-read-args
           (concat "Query replace"
                   (and current-prefix-arg  " word")
                   (and transient-mark-mode  mark-active
                        (not search/replace-region-as-default-flag)
                        " in region"))
           nil)))
     (list (nth 0 common) (nth 1 common) (nth 2 common)
           (and transient-mark-mode  mark-active  (region-beginning))
           (and transient-mark-mode  mark-active  (region-end))))))
 
Then C-h f query-replace.  You see this as part of the description:
 
 :around advice: `ad-Advice-query-replace'
 
1. It should be :before, not :around, no?
(When you click on that link it confirms that it is `Before-advice'.)
 
2. How can you tell where that advice is defined?  It doesn't matter
whether you put that defadvice in a file foo.el and load that.  Neither
C-h f query-replace nor clicking `ad-Advice-query-replace' in that
description tells you anything about where the actual advice
`respect-search/replace-region-as-default-flag' is defined.


In GNU Emacs 24.3.50.1 (i386-mingw-nt5.1.2600)
 of 2013-03-23 on VBOX
Bzr revision: 112115 eliz <at> gnu.org-20130323093300-rjs0dgskxm9u0ya4
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -IC:/emacs/libs/libXpm-3.5.10/include -IC:/emacs/libs/libXpm-3.5.10/src
 -IC:/emacs/libs/libpng-dev_1.4.3-1_win32/include
 -IC:/emacs/libs/zlib-dev_1.2.5-2_win32/include
 -IC:/emacs/libs/giflib-4.1.4-1-lib/include
 -IC:/emacs/libs/jpeg-6b-4-lib/include
 -IC:/emacs/libs/tiff-3.8.2-1-lib/include
 -IC:/emacs/libs/libxml2-2.7.8-w32-bin/include/libxml2
 -IC:/emacs/libs/gnutls-3.1.10-w32/include
 -IC:/emacs/libs/libiconv-1.14-2-mingw32-dev/include'
 

Message #8 received at 14070 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 14070 <at> debbugs.gnu.org
Subject: Re: bug#14070: 24.3.50;
	incorrect doc from `C-h f' when use `defadvice' with `before'
Date: Wed, 27 Mar 2013 15:19:42 -0400

tags 14070 notabug
thanks

>  :around advice: `ad-Advice-query-replace'
> 1. It should be :before, not :around, no?
> (When you click on that link it confirms that it is `Before-advice'.)

ad-Advice-query-replace is an "advice container" which in turns contains
all the advice added via `defadvice'.

> 2. How can you tell where that advice is defined?

You can't because defadvice does not record this information.
If you replace your advice with something like:

   (advice-add 'query-replace :before
               #'respect-search/replace-region-as-default-flag)
   (defun respect-search/replace-region-as-default-flag (&rest ignore)
     (interactive
      (let ((common
             (query-replace-read-args
              (concat "Query replace"
                      (and current-prefix-arg  " word")
                      (and transient-mark-mode  mark-active
                           (not search/replace-region-as-default-flag)
                           " in region"))
              nil)))
        (list (nth 0 common) (nth 1 common) (nth 2 common)
              (and transient-mark-mode  mark-active  (region-beginning))
              (and transient-mark-mode  mark-active  (region-end)))))
     nil)

then C-h f should tell you it's a before advice and should let you click
your way to its source code.


        Stefan

Message #13 received at 14070 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>
Cc: 14070 <at> debbugs.gnu.org
Subject: RE: bug#14070: 24.3.50;
	incorrect doc from `C-h f' when use `defadvice' with `before'
Date: Wed, 27 Mar 2013 12:39:48 -0700

> > :around advice: `ad-Advice-query-replace'
> > 1. It should be :before, not :around, no?
> > (When you click on that link it confirms that it is 
> > `Before-advice'.)
> 
> ad-Advice-query-replace is an "advice container" which in 
> turns contains all the advice added via `defadvice'.

Maybe so, but the point is that the description in `C-h f' is unclear.  The
occurrence of `:around', in particular (and why show a keyword here?) can
mislead.

If you cannot say anything here about what kinds of advice are currently
included in this container, then at least remove the :around - there is no
around advice in the recipe I gave.

The bug is about improving the doc displayed to users.  Please consider
reopening it and untagging it `notabug', until such improvement has actually
been made.

> > 2. How can you tell where that advice is defined?
> 
> You can't because defadvice does not record this information.

3. (Let me know if you prefer a separate enhancement request for this.)

So how about fixing that?

> If you replace your advice with something like:
> 
>    (advice-add 'query-replace :before
>                #'respect-search/replace-region-as-default-flag)

No such function, up through Emacs 24.3: `advice-add'.

Yes, it exists in the version I filed the bug for.  But I am not interested only
in that version.  And I am also interested in defadvice - which is still
supported AFAIK.

>    (defun respect-search/replace-region-as-default-flag (&rest ignore)
>      (interactive...)
>      nil)
> then C-h f should tell you it's a before advice and should 
> let you click your way to its source code.

Sounds good.

4. (Let me know if you prefer a separate bug report for this.)

`advice-add' should have face `font-lock-keyword-face', like `defun' etc.
And the advised function should have face `font-lock-function-name-face'.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.