bug#17348: 24.3.50; Cannot advice-add the same function :before and :after

bug#17348: 24.3.50; Cannot advice-add the same function :before and :after

[Nicolas Richard]

Evalling:
(progn
  (defun yfcheck () (message "Check"))
  (defun yftest () (message "Test"))
  (advice-add 'yftest :before #'yfcheck)
  (advice-add 'yftest :after #'yfcheck)
  (yftest)
  nil)

gives (in the *Messages* buffer)
Test
Check

I'd have expected:
Check
Test
Check

-- 
Nico.

bug#17348: 24.3.50; Cannot advice-add the same function :before and :after

[Stefan Monnier]

>   (advice-add 'yftest :before #'yfcheck)
>   (advice-add 'yftest :after #'yfcheck)

`advice-add' will not add the same function twice, indeed.
Similarly to `add-hook'.  This is done explicitly in advice--add-function:

    (when a
      ;; The advice is already present.  Remove the old one, first.
      (setf (gv-deref ref)
            (advice--remove-function (gv-deref ref) (advice--car a))))

I guess we could change that, but then the notion of "removal" becomes
more complicated.


        Stefan

bug#17348: 24.3.50; Cannot advice-add the same function :before and :after

[Nicolas Richard]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>   (advice-add 'yftest :before #'yfcheck)
>>   (advice-add 'yftest :after #'yfcheck)
>
> `advice-add' will not add the same function twice, indeed.
> Similarly to `add-hook'.  This is done explicitly in advice--add-function:
>
>     (when a
>       ;; The advice is already present.  Remove the old one, first.
>       (setf (gv-deref ref)
>             (advice--remove-function (gv-deref ref) (advice--car a))))
>
> I guess we could change that, but then the notion of "removal" becomes
> more complicated.

In the actual situation that lead me to adding the same function twice,
it was a tracing function, like (message "time: %s ; value of
foovariable: %s" (current-time) foovariable) and I assumed that I would
be able to remove them both with one (advice-remove ...) -- I guess that
isn't too complicated.

OTOH I also think that the current behaviour is fine, but I'd suggest
emitting a warning when an advice is being overwritten/removed that way.

-- 
Nico.

bug#17348: 24.3.50; Cannot advice-add the same function :before and :after

[Stefan Monnier]

> In the actual situation that lead me to adding the same function twice,
> it was a tracing function, like (message "time: %s ; value of
> foovariable: %s" (current-time) foovariable) and I assumed that I would
> be able to remove them both with one (advice-remove ...) -- I guess that
> isn't too complicated.

Removing them both would not be desirable in general.

> OTOH I also think that the current behaviour is fine, but I'd suggest
> emitting a warning when an advice is being overwritten/removed that way.

It's not meant to be a "secret" behavior, so people should be able to
rely on this behavior (just like you can rely on add-hook being
idempotent) and use it, in which case emitting a warning is
not appropriate.

This said, maybe we should make it easier to do what you wanted.
E.g. we could probably make it possible to use different `name'
properties to let a user add a function several times (potentially at
different places, but even at the same place).


        Stefan

bug#17348: 24.3.50; Cannot advice-add the same function :before and :after

[Stefan Monnier]

I installed a patch in `emacs-24' which will let you add the same
function multiple times, as long as you specify different `name'
properties for each one.

>> In the actual situation that lead me to adding the same function twice,
>> it was a tracing function, like (message "time: %s ; value of
>> foovariable: %s" (current-time) foovariable) and I assumed that I would
>> be able to remove them both with one (advice-remove ...) -- I guess that
>> isn't too complicated.
> Removing them both would not be desirable in general.

With the new code, you can remove each one individually by specifying
its `name', or you can remove them all by specifying the function itself.


        Stefan