Last use of defadvice in Emacs

Last use of defadvice in Emacs

[Stefan Monnier]

Hi Alan,

The patch below replaces the last remaining use of `defadvice` in Emacs
(well, except for Org where this has already been fixed upstream but
we're waiting for the change to trickle down to `master`).

It's guaranteed 100% untested, tho.

It's kinda weird in that this `defadvice` is not actually used in
current Emacsen, but we still have to macro-expand it, so the new code
macroexpands to the corresponding use of `advice-add` even tho that also
won't be used.


        Stefan


diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
index 957a0b8a7c5..759a01f1dd8 100644
--- a/lisp/progmodes/cc-mode.el
+++ b/lisp/progmodes/cc-mode.el
@@ -2563,14 +2563,18 @@ c-extend-after-change-region
   (cons c-new-BEG c-new-END))
 
 ;; Emacs < 22 and XEmacs
+(defun c--fl-extend-region (args)
+  ;; Make sure that any string/regexp is completely font-locked.
+  (if (not c-buffer-is-cc-mode)
+      args
+    (list c-new-BEG c-new-END (cddr args))))
+
 (defmacro c-advise-fl-for-region (function)
   (declare (debug t))
-  `(defadvice ,function (before get-awk-region activate)
-     ;; Make sure that any string/regexp is completely font-locked.
-     (when c-buffer-is-cc-mode
-       (save-excursion
-	 (ad-set-arg 1 c-new-END)   ; end
-	 (ad-set-arg 0 c-new-BEG)))))	; beg
+  (if (fboundp 'advice-add)
+      `(advice-add ',function :filter-args #'c--fl-extend-region)
+    `(defadvice ,function (before get-awk-region activate)
+       (ad-set-args 0 (c--fl-extend-region (ad-get-args 0))))))
 
 (unless (boundp 'font-lock-extend-after-change-region-function)
   (c-advise-fl-for-region font-lock-after-change-function)

Re: Last use of defadvice in Emacs

[T.V Raman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 2024 bytes --]

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


I assume it's still okay and supported for external Emacs packages to
use defadvice.
> Hi Alan,
>
> The patch below replaces the last remaining use of `defadvice` in Emacs
> (well, except for Org where this has already been fixed upstream but
> we're waiting for the change to trickle down to `master`).
>
> It's guaranteed 100% untested, tho.
>
> It's kinda weird in that this `defadvice` is not actually used in
> current Emacsen, but we still have to macro-expand it, so the new code
> macroexpands to the corresponding use of `advice-add` even tho that also
> won't be used.
>
>
>         Stefan
>
>
> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
> index 957a0b8a7c5..759a01f1dd8 100644
> --- a/lisp/progmodes/cc-mode.el
> +++ b/lisp/progmodes/cc-mode.el
> @@ -2563,14 +2563,18 @@ c-extend-after-change-region
>    (cons c-new-BEG c-new-END))
>  
>  ;; Emacs < 22 and XEmacs
> +(defun c--fl-extend-region (args)
> +  ;; Make sure that any string/regexp is completely font-locked.
> +  (if (not c-buffer-is-cc-mode)
> +      args
> +    (list c-new-BEG c-new-END (cddr args))))
> +
>  (defmacro c-advise-fl-for-region (function)
>    (declare (debug t))
> -  `(defadvice ,function (before get-awk-region activate)
> -     ;; Make sure that any string/regexp is completely font-locked.
> -     (when c-buffer-is-cc-mode
> -       (save-excursion
> -	 (ad-set-arg 1 c-new-END)   ; end
> -	 (ad-set-arg 0 c-new-BEG)))))	; beg
> +  (if (fboundp 'advice-add)
> +      `(advice-add ',function :filter-args #'c--fl-extend-region)
> +    `(defadvice ,function (before get-awk-region activate)
> +       (ad-set-args 0 (c--fl-extend-region (ad-get-args 0))))))
>  
>  (unless (boundp 'font-lock-extend-after-change-region-function)
>    (c-advise-fl-for-region font-lock-after-change-function)
>
>

-- 

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
7©4 Id: kg:/m/0285kf1  •0Ü8

Re: Last use of defadvice in Emacs

[Stefan Monnier via CC-Mode-help]

> I assume it's still okay and supported for external Emacs packages to
> use defadvice.

Of course: it's not even declared obsolete, AFAIK.


        Stefan

Re: Last use of defadvice in Emacs

[T.V Raman]

Thanks, that's a relief.


Stefan Monnier writes:
 > > I assume it's still okay and supported for external Emacs packages to
 > > use defadvice.
 > 
 > Of course: it's not even declared obsolete, AFAIK.
 > 
 > 
 >         Stefan

-- 

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
♉ Id: kg:/m/0285kf1  🦮

--

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
♉ Id: kg:/m/0285kf1  🦮

Re: Last use of defadvice in Emacs

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Since using advising within Emacs makes maintenance harder, It would
be good to systematically eliminate all uses of `advice-add' in Emacs
core, and create new hook variables as needed to handle the jobs.



-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Last use of defadvice in Emacs

[Alan Mackenzie]

Hello, Stefan.

On Mon, Apr 04, 2022 at 15:49:16 -0400, Stefan Monnier wrote:
> Hi Alan,

> The patch below replaces the last remaining use of `defadvice` in Emacs
> (well, except for Org where this has already been fixed upstream but
> we're waiting for the change to trickle down to `master`).

Why would we want to replace defadvice with advice-add?  Don't all the
objections to advice apply equally to both forms?

> It's guaranteed 100% untested, tho.

:-)

> It's kinda weird in that this `defadvice` is not actually used in
> current Emacsen, but we still have to macro-expand it, so the new code
> macroexpands to the corresponding use of `advice-add` even tho that also
> won't be used.

Yes, Emacs 21 was a long time ago.  Even CC Mode doesn't support it any
more.  The only thing which still uses the advice is XEmacs, and I don't
thing that has advice-add.  So I'm not sure the patch in the current
form makes much sense.  It might be better just to remove the < Emacs 22
bit altogether from the Emacs version of CC Mode.

I've spent an hour trying various combinations of eval-when-compile and
(boundp 'font-lock-extend-after-change-region-function), to try and get
the stuff compiled or ignored based on the presence/absence of that
variable.  Something like C's #ifdef.  I didn't manage it, and don't
think it's possible.  That's another C facility Emacs Lisp seems to be
missing.

>         Stefan


> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
> index 957a0b8a7c5..759a01f1dd8 100644
> --- a/lisp/progmodes/cc-mode.el
> +++ b/lisp/progmodes/cc-mode.el
> @@ -2563,14 +2563,18 @@ c-extend-after-change-region
>    (cons c-new-BEG c-new-END))
 
>  ;; Emacs < 22 and XEmacs
> +(defun c--fl-extend-region (args)
> +  ;; Make sure that any string/regexp is completely font-locked.
> +  (if (not c-buffer-is-cc-mode)
> +      args
> +    (list c-new-BEG c-new-END (cddr args))))
> +
>  (defmacro c-advise-fl-for-region (function)
>    (declare (debug t))
> -  `(defadvice ,function (before get-awk-region activate)
> -     ;; Make sure that any string/regexp is completely font-locked.
> -     (when c-buffer-is-cc-mode
> -       (save-excursion
> -	 (ad-set-arg 1 c-new-END)   ; end
> -	 (ad-set-arg 0 c-new-BEG)))))	; beg
> +  (if (fboundp 'advice-add)
> +      `(advice-add ',function :filter-args #'c--fl-extend-region)
> +    `(defadvice ,function (before get-awk-region activate)
> +       (ad-set-args 0 (c--fl-extend-region (ad-get-args 0))))))
 
>  (unless (boundp 'font-lock-extend-after-change-region-function)
>    (c-advise-fl-for-region font-lock-after-change-function)

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Last use of defadvice in Emacs

[Stefan Monnier via CC-Mode-help]

>> The patch below replaces the last remaining use of `defadvice` in Emacs
>> (well, except for Org where this has already been fixed upstream but
>> we're waiting for the change to trickle down to `master`).
>
> Why would we want to replace defadvice with advice-add?
> Don't all the objections to advice apply equally to both forms?

Both should be avoided, indeed.  But `defadvice` is slowly being
replaced because it cannot obey `lexical-binding` (along with a bunch
of more minor annoyances) so it gets a few more objections.

> I've spent an hour trying various combinations of eval-when-compile and
> (boundp 'font-lock-extend-after-change-region-function), to try and get
> the stuff compiled or ignored based on the presence/absence of that
> variable.  Something like C's #ifdef.  I didn't manage it, and don't
> think it's possible.  That's another C facility Emacs Lisp seems to be
> missing.

I thought the code was written specifically to perform the test at
runtime, so that the code compiled on Emacs-21 would still skip the
advice when run on Emacs≥22 (and also so code compiled on Emacs-28 would
still activate the advice when run on some hypothetical future Emacs
where `font-lock-extend-after-change-region-function` has been removed).

If you want to perform the test at compile time then I think something
like the following should work.


        Stefan


diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
index 957a0b8a7c5..05da61dbb2f 100644
--- a/lisp/progmodes/cc-mode.el
+++ b/lisp/progmodes/cc-mode.el
@@ -2565,18 +2565,18 @@
 ;; Emacs < 22 and XEmacs
 (defmacro c-advise-fl-for-region (function)
   (declare (debug t))
+  (unless (boundp 'font-lock-extend-after-change-region-function)
   `(defadvice ,function (before get-awk-region activate)
      ;; Make sure that any string/regexp is completely font-locked.
      (when c-buffer-is-cc-mode
        (save-excursion
 	 (ad-set-arg 1 c-new-END)   ; end
-	 (ad-set-arg 0 c-new-BEG)))))	; beg
+	   (ad-set-arg 0 c-new-BEG)))))) ; beg
 
-(unless (boundp 'font-lock-extend-after-change-region-function)
-  (c-advise-fl-for-region font-lock-after-change-function)
-  (c-advise-fl-for-region jit-lock-after-change)
-  (c-advise-fl-for-region lazy-lock-defer-rest-after-change)
-  (c-advise-fl-for-region lazy-lock-defer-line-after-change))
+(c-advise-fl-for-region font-lock-after-change-function)
+(c-advise-fl-for-region jit-lock-after-change)
+(c-advise-fl-for-region lazy-lock-defer-rest-after-change)
+(c-advise-fl-for-region lazy-lock-defer-line-after-change)
 
 ;; Connect up to `electric-indent-mode' (Emacs 24.4 and later).
 (defun c-electric-indent-mode-hook ()

Re: Last use of defadvice in Emacs

[T.V Raman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 3730 bytes --]

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


Hi Stefan,

Could you give some tips on what  
issues one might see with defadvice and it's disrespect of lexical
bindings?

For the record, I switched emacspeak to lexical binding a couple of
years ago and fixed any/all bytecomp warnings it generated --- and I've
not hit any bugs to date. But it would be helpful to understand the
types of issues defadvice might cause with lexical binding if something
inexplicable does show up.

This next is likely not lexical binding related, but the only issue I
needed to create an emacspeak specific check was for the equivalent of
(called-interactively 'interactive) which exhibited strange behavior
with defadvice -- I created a "tighter" check that is
emacspeak-specific.


>>> The patch below replaces the last remaining use of `defadvice` in Emacs
>>> (well, except for Org where this has already been fixed upstream but
>>> we're waiting for the change to trickle down to `master`).
>>
>> Why would we want to replace defadvice with advice-add?
>> Don't all the objections to advice apply equally to both forms?
>
> Both should be avoided, indeed.  But `defadvice` is slowly being
> replaced because it cannot obey `lexical-binding` (along with a bunch
> of more minor annoyances) so it gets a few more objections.
>
>> I've spent an hour trying various combinations of eval-when-compile and
>> (boundp 'font-lock-extend-after-change-region-function), to try and get
>> the stuff compiled or ignored based on the presence/absence of that
>> variable.  Something like C's #ifdef.  I didn't manage it, and don't
>> think it's possible.  That's another C facility Emacs Lisp seems to be
>> missing.
>
> I thought the code was written specifically to perform the test at
> runtime, so that the code compiled on Emacs-21 would still skip the
> advice when run on Emacs¡Ý22 (and also so code compiled on Emacs-28 would
> still activate the advice when run on some hypothetical future Emacs
> where `font-lock-extend-after-change-region-function` has been removed).
>
> If you want to perform the test at compile time then I think something
> like the following should work.
>
>
>         Stefan
>
>
> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
> index 957a0b8a7c5..05da61dbb2f 100644
> --- a/lisp/progmodes/cc-mode.el
> +++ b/lisp/progmodes/cc-mode.el
> @@ -2565,18 +2565,18 @@
>  ;; Emacs < 22 and XEmacs
>  (defmacro c-advise-fl-for-region (function)
>    (declare (debug t))
> +  (unless (boundp 'font-lock-extend-after-change-region-function)
>    `(defadvice ,function (before get-awk-region activate)
>       ;; Make sure that any string/regexp is completely font-locked.
>       (when c-buffer-is-cc-mode
>         (save-excursion
>  	 (ad-set-arg 1 c-new-END)   ; end
> -	 (ad-set-arg 0 c-new-BEG)))))	; beg
> +	   (ad-set-arg 0 c-new-BEG)))))) ; beg
>  
> -(unless (boundp 'font-lock-extend-after-change-region-function)
> -  (c-advise-fl-for-region font-lock-after-change-function)
> -  (c-advise-fl-for-region jit-lock-after-change)
> -  (c-advise-fl-for-region lazy-lock-defer-rest-after-change)
> -  (c-advise-fl-for-region lazy-lock-defer-line-after-change))
> +(c-advise-fl-for-region font-lock-after-change-function)
> +(c-advise-fl-for-region jit-lock-after-change)
> +(c-advise-fl-for-region lazy-lock-defer-rest-after-change)
> +(c-advise-fl-for-region lazy-lock-defer-line-after-change)
>  
>  ;; Connect up to `electric-indent-mode' (Emacs 24.4 and later).
>  (defun c-electric-indent-mode-hook ()
>
>

-- 

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
7©4 Id: kg:/m/0285kf1  •0Ü8

Re: Last use of defadvice in Emacs

[Stefan Monnier via CC-Mode-help]

> Could you give some tips on what issues one might see with defadvice
> and its disrespect of lexical bindings?

IIRC for existing pieces of advice the main issue can occur if an advice
accesses the formal arguments by their name rather than via things like
`ad-get-arg`.  IIRC we managed to get it working like it used to in most
cases, but it's not very robust.

The other issue is that the body of `defadvice` is always evaluated in
the dynamically scoped dialect of ELisp.  This means that its local vars
can "leak" to some of the functions they call (as is normal with
dynamic scoping) and that they can't refer to surrounding lexical
variables (more common with `ad-make-advice` than with `defadvice` since
that one is usually used at top-level), ...

Those problems aren't new.  What's "new" is that `advice-add` doesn't
suffer from them.

> For the record, I switched emacspeak to lexical binding a couple of
> years ago and fixed any/all bytecomp warnings it generated --- and
> I've not hit any bugs to date.

That's indeed how it often turns out :-)
[ Sadly, not all problems are caught by warnings, but the vast
  majority.  ]

> But it would be helpful to understand the types of issues defadvice
> might cause with lexical binding if something inexplicable does
> show up.

For people who grew up on dynamically scoped ELisp, there are usually no
bad surprises.  The surprises come up when you take lexical scoping as
a given.

> This next is likely not lexical binding related, but the only issue I
> needed to create an emacspeak specific check was for the equivalent of
> (called-interactively 'interactive) which exhibited strange behavior
> with defadvice -- I created a "tighter" check that is
> emacspeak-specific.

`called-interactively-p` is inherently brittle.  We go through some
trouble to make it work "right" (this is sometimes hard to define) for
as many cases as possible, but it's fundamentally an impossible job.
Sadly, it's particularly hard to do for pieces of advice yet also
particular useful there :-(


        Stefan


>>>> The patch below replaces the last remaining use of `defadvice` in Emacs
>>>> (well, except for Org where this has already been fixed upstream but
>>>> we're waiting for the change to trickle down to `master`).
>>>
>>> Why would we want to replace defadvice with advice-add?
>>> Don't all the objections to advice apply equally to both forms?
>>
>> Both should be avoided, indeed.  But `defadvice` is slowly being
>> replaced because it cannot obey `lexical-binding` (along with a bunch
>> of more minor annoyances) so it gets a few more objections.
>>
>>> I've spent an hour trying various combinations of eval-when-compile and
>>> (boundp 'font-lock-extend-after-change-region-function), to try and get
>>> the stuff compiled or ignored based on the presence/absence of that
>>> variable.  Something like C's #ifdef.  I didn't manage it, and don't
>>> think it's possible.  That's another C facility Emacs Lisp seems to be
>>> missing.
>>
>> I thought the code was written specifically to perform the test at
>> runtime, so that the code compiled on Emacs-21 would still skip the
>> advice when run on Emacs≥22 (and also so code compiled on Emacs-28 would
>> still activate the advice when run on some hypothetical future Emacs
>> where `font-lock-extend-after-change-region-function` has been removed).
>>
>> If you want to perform the test at compile time then I think something
>> like the following should work.
>>
>>
>>         Stefan
>>
>>
>> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
>> index 957a0b8a7c5..05da61dbb2f 100644
>> --- a/lisp/progmodes/cc-mode.el
>> +++ b/lisp/progmodes/cc-mode.el
>> @@ -2565,18 +2565,18 @@
>>  ;; Emacs < 22 and XEmacs
>>  (defmacro c-advise-fl-for-region (function)
>>    (declare (debug t))
>> +  (unless (boundp 'font-lock-extend-after-change-region-function)
>>    `(defadvice ,function (before get-awk-region activate)
>>       ;; Make sure that any string/regexp is completely font-locked.
>>       (when c-buffer-is-cc-mode
>>         (save-excursion
>>  	 (ad-set-arg 1 c-new-END)   ; end
>> -	 (ad-set-arg 0 c-new-BEG)))))	; beg
>> +	   (ad-set-arg 0 c-new-BEG)))))) ; beg
>>  
>> -(unless (boundp 'font-lock-extend-after-change-region-function)
>> -  (c-advise-fl-for-region font-lock-after-change-function)
>> -  (c-advise-fl-for-region jit-lock-after-change)
>> -  (c-advise-fl-for-region lazy-lock-defer-rest-after-change)
>> -  (c-advise-fl-for-region lazy-lock-defer-line-after-change))
>> +(c-advise-fl-for-region font-lock-after-change-function)
>> +(c-advise-fl-for-region jit-lock-after-change)
>> +(c-advise-fl-for-region lazy-lock-defer-rest-after-change)
>> +(c-advise-fl-for-region lazy-lock-defer-line-after-change)
>>  
>>  ;; Connect up to `electric-indent-mode' (Emacs 24.4 and later).
>>  (defun c-electric-indent-mode-hook ()
>>
>>

Re: Last use of defadvice in Emacs

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Alan Mackenzie <acm@muc.de>,  bug-cc-mode@gnu.org,  emacs-devel@gnu.org
> Date: Wed, 06 Apr 2022 22:49:08 -0400
> 
> > Could you give some tips on what issues one might see with defadvice
> > and its disrespect of lexical bindings?
> 
> IIRC for existing pieces of advice the main issue can occur if an advice
> accesses the formal arguments by their name rather than via things like
> `ad-get-arg`.  IIRC we managed to get it working like it used to in most
> cases, but it's not very robust.
> 
> The other issue is that the body of `defadvice` is always evaluated in
> the dynamically scoped dialect of ELisp.  This means that its local vars
> can "leak" to some of the functions they call (as is normal with
> dynamic scoping) and that they can't refer to surrounding lexical
> variables (more common with `ad-make-advice` than with `defadvice` since
> that one is usually used at top-level), ...
> 
> Those problems aren't new.  What's "new" is that `advice-add` doesn't
> suffer from them.

It would be nice if these issues (perhaps in less academic shape and
in more practical terms) could be added to the ELisp manual, because
AFAICT we currently lack any rationale for people not to use
defadvice.

Re: Last use of defadvice in Emacs

[Stefan Monnier via CC-Mode-help]

> It would be nice if these issues (perhaps in less academic shape and
> in more practical terms) could be added to the ELisp manual,

Not sure what "more practical terms" would look like.

> because AFAICT we currently lack any rationale for people not to
> use defadvice.

BTW, in https://emacs.stackexchange.com/questions/3079/#3102
I mention a few other reasons, such as:

    • Design simplicity: defadvice has various notions that are
    generally difficult to understand precisely and/or rarely used.
    E.g. the difference between "enabling" and "activating" advices.
    Or the meaning of "pre" and/or "compiled".  There are also quirks in
    the handling of `ad-do-it`, such as the fact that it looks like
    a variable-reference rather than a call, or the fact that you need
    to (setq ad-return-value ...) explicitly rather than simply
    returning the value.

    • Defadvice suffers from various problems w.r.t macroexpansion and
    compilation: the body of an advice is not exposed as "code" (which
    the compiler and macroexpander see) but as "data" which is later on
    combined to make up an expression.  So macroexpansion happens late
    (which can causes surprises if you use things like
    `(eval-when-compile (require 'foo))`) and lexical-scoping is hard to
    preserve correctly.

E.g. regarding the first point, `ad-activate` or `ad-deactivate` are
arguably misdesigns since they have a global effect (they affect all
pieces of advice applied to a given function) and most uses of them are
latent bugs.

This said, I don't know how important it is to document those
advantages: AFAICT all coders familiar with the existence of both prefer
using `advice-add` already.  For new users who know neither, the doc
already nudges them towards `advice-add`.  Remains those who just
copy&paste existing code over which the doc has no influence, and more
importantly the vast number of existing code using `defadvice` for which
rewriting can't bring too many immediate benefit since the code
already works and we'd be lying if we tried to claim the rewritten code
is faster or objectively better.

To me the main benefit is that `advice-add` is simpler both in
implementation and in API (no need to learn about `add-(de)activate`,
`ad-(g|s)et-arg(s)`, `ad-do-it`, `ad-(en|dis)able-advice`, the
differences between "adding", "enabling", and "activating", the
occasional need to figure how and when to compile the code, ...).

Compare the size and complexity of Emacs's `advice.el` (which implements
the `defadvice` API on top of the `nadvice.el` one) to that of GNU
ELPA's `nadvice.el` (which implements the `advice-add` API on top of
`advice.el`): it's about 35× larger, yet both APIs cover adequately the
actual needs of packages.


        Stefan

Re: Last use of defadvice in Emacs

[T.V Raman]

Stefan, Thank you very much for this.

Couple of items:

1. ad-get-arg vs ad-get-argument: I've always used ad-get-arg --
   however for the last few years I've notied that ad-get-arg is
   getting harder and harder to discover; for the last few years,
   emacs always completes ad-get-arg to ad-get-argument.  I personally
   find ad-get-arg to be more idiomatic and easier to use.

   2. Re ad-return-value -- I've almost never had to explicitly set
      ad-return-value -- in around and after advice, I have always
      returned ad-return-value and have never hit issues --- perhaps
      because I've always wrappered my advice body in a let form --
      not sure. What kind of breakages happen when returning
      ad-return-value vs explicitly setting ad-return-value at the end
      of the advice body?

      3. I've avoided the complexity around preactivation vs
         activation etc by always saying (... pre act comp) in all my
         advice forms and also making sure to name all my advices (in
         my case emacspeak) so one can tell where the advice came
         from.

         
-- 

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
♉ Id: kg:/m/0285kf1  🦮

--

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
♉ Id: kg:/m/0285kf1  🦮

Re: Last use of defadvice in Emacs

[Stefan Monnier via CC-Mode-help]

> 1. ad-get-arg vs ad-get-argument: I've always used ad-get-arg --
>    however for the last few years I've notied that ad-get-arg is
>    getting harder and harder to discover; for the last few years,
>    emacs always completes ad-get-arg to ad-get-argument.  I personally
>    find ad-get-arg to be more idiomatic and easier to use.

`ad-get-argument` is basically an internal function of `advice.el` so
you never want to use it.  But `ad-get-arg` (the thing you *do* need to
use to access arguments) is not "defined" anywhere, neither as
a function nor as a macro, so TAB completion will never know (and has
never known) to use it.

For kicks, try an advice like

    (defadvice next-line (before some-fun activate)
      (message "surprise: %S" '(ad-get-args 0)))


> 2. Re ad-return-value -- I've almost never had to explicitly set
>    ad-return-value -- in around and after advice, I have always
>    returned ad-return-value and have never hit issues --- perhaps
>    because I've always wrappered my advice body in a let form --
>    not sure. What kind of breakages happen when returning
>    ad-return-value vs explicitly setting ad-return-value at the end
>    of the advice body?

You typically need to set it when you want to do:

    (defadvice some-function (around my-advice activate)
      (if (bla bla)
          (cons 1 ad-do-it)
        (do something else)
        (and return some value)))

which needs to be something like:

    (defadvice some-function (around my-advice activate)
      (if (bla bla)
          (progn ad-do-it
            (push 1 ad-return-value))
        (do something else)
        (setq ad-return-value (and return some value))))

>       3. I've avoided the complexity around preactivation vs
>          activation etc by always saying (... pre act comp) in all my
>          advice forms

Not sure in which sense this "avoided the complexity".
And most people reading your code won't know what it means, I'm afraid.


        Stefan

Re: Last use of defadvice in Emacs

[T.V Raman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 2091 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
Relevant pieces retained alongside responses:

>> 2. Re ad-return-value -- I've almost never had to explicitly set
>>    ad-return-value -- in around and after advice, I have always
>>    returned ad-return-value and have never hit issues --- perhaps
>>    because I've always wrappered my advice body in a let form --
This has never bitten emacspeak because I always leave the return value
>>    unmolested, and for safely always make sure that ad-return-value
>>    is the last form in the advice body so that functions or code that
>>    runs after the advice doesn't break because the original
>>    function's return value got dropped along the way 


>>    not sure. What kind of breakages happen when returning
>>    ad-return-value vs explicitly setting ad-return-value at the end
>>    of the advice body?
>
> You typically need to set it when you want to do:
>
>     (defadvice some-function (around my-advice activate)
>       (if (bla bla)
>           (cons 1 ad-do-it)
>         (do something else)
>         (and return some value)))
>
> which needs to be something like:
>
>     (defadvice some-function (around my-advice activate)
>       (if (bla bla)
>           (progn ad-do-it
>             (push 1 ad-return-value))
>         (do something else)
>         (setq ad-return-value (and return some value))))
>
>>       3. I've avoided the complexity around preactivation vs
>>          activation etc by always saying (... pre act comp) in all my

Avoid complexity here == "my little brain doesn't have to worry about
it" --- all the calls to defadvice in emacspeak say "pre act comp"
which means if it needs changing, I can change them all with a single
line of perl or sed magic.

>>          advice forms
>
> Not sure in which sense this "avoided the complexity".
> And most people reading your code won't know what it means, I'm afraid.
>
>
>         Stefan
>
>

-- 

Thanks,

--Raman(I Search, I Find, I Misplace, I Research)
7©4 Id: kg:/m/0285kf1  •0Ü8

Re: Last use of defadvice in Emacs

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: raman@google.com,  acm@muc.de,  bug-cc-mode@gnu.org,  emacs-devel@gnu.org
> Date: Thu, 07 Apr 2022 17:59:41 -0400
> 
> > It would be nice if these issues (perhaps in less academic shape and
> > in more practical terms) could be added to the ELisp manual,
> 
> Not sure what "more practical terms" would look like.

Something that makes it clear why using defadvice with lexical-binding
will cause trouble: an explanation with one or two examples.

> BTW, in https://emacs.stackexchange.com/questions/3079/#3102
> I mention a few other reasons, such as:
> 
>     • Design simplicity: defadvice has various notions that are
>     generally difficult to understand precisely and/or rarely used.
>     E.g. the difference between "enabling" and "activating" advices.
>     Or the meaning of "pre" and/or "compiled".  There are also quirks in
>     the handling of `ad-do-it`, such as the fact that it looks like
>     a variable-reference rather than a call, or the fact that you need
>     to (setq ad-return-value ...) explicitly rather than simply
>     returning the value.
> 
>     • Defadvice suffers from various problems w.r.t macroexpansion and
>     compilation: the body of an advice is not exposed as "code" (which
>     the compiler and macroexpander see) but as "data" which is later on
>     combined to make up an expression.  So macroexpansion happens late
>     (which can causes surprises if you use things like
>     `(eval-when-compile (require 'foo))`) and lexical-scoping is hard to
>     preserve correctly.
> 
> E.g. regarding the first point, `ad-activate` or `ad-deactivate` are
> arguably misdesigns since they have a global effect (they affect all
> pieces of advice applied to a given function) and most uses of them are
> latent bugs.

This, too, is important to know, IMO.

> This said, I don't know how important it is to document those
> advantages

Well, I obviously think it is, which is why I asked to do that,
please.

> To me the main benefit is that `advice-add` is simpler both in
> implementation and in API (no need to learn about `add-(de)activate`,
> `ad-(g|s)et-arg(s)`, `ad-do-it`, `ad-(en|dis)able-advice`, the
> differences between "adding", "enabling", and "activating", the
> occasional need to figure how and when to compile the code, ...).

Perhaps, this should also be mentioned in the manual.

Thanks.

Re: Last use of defadvice in Emacs

[Alan Mackenzie]

Hello, Stefan.

On Wed, Apr 06, 2022 at 17:08:21 -0400, Stefan Monnier wrote:
> >> The patch below replaces the last remaining use of `defadvice` in Emacs
> >> (well, except for Org where this has already been fixed upstream but
> >> we're waiting for the change to trickle down to `master`).

> > Why would we want to replace defadvice with advice-add?
> > Don't all the objections to advice apply equally to both forms?

> Both should be avoided, indeed.  But `defadvice` is slowly being
> replaced because it cannot obey `lexical-binding` (along with a bunch
> of more minor annoyances) so it gets a few more objections.

Ah, OK.

> > I've spent an hour trying various combinations of eval-when-compile and
> > (boundp 'font-lock-extend-after-change-region-function), to try and get
> > the stuff compiled or ignored based on the presence/absence of that
> > variable.  Something like C's #ifdef.  I didn't manage it, and don't
> > think it's possible.  That's another C facility Emacs Lisp seems to be
> > missing.

> I thought the code was written specifically to perform the test at
> runtime, so that the code compiled on Emacs-21 would still skip the
> advice when run on Emacs≥22 (and also so code compiled on Emacs-28 would
> still activate the advice when run on some hypothetical future Emacs
> where `font-lock-extend-after-change-region-function` has been removed).

Maybe it was.  It's such a long time ago, I can't remember.

> If you want to perform the test at compile time then I think something
> like the following should work.

I want more than to "perform the test" at compile time.  I want a Lisp
form that will check whether that variable is bound, and if so, not even
compile the sub-form.  Something like C's #ifndef preprocessor form.  It
would look something like

    (hash-if (not
              (boundp 'font-lock-extend-after-change-region-function))
             (progn
	      (defmacro c-advise-fl-for-region (function) .....)
              (c-advise-fl-for-region ....)
	      ....
	      ))

..  Here the progn form would be neither evaluated nor compiled if that
font-lock-... variable were boundp.  We don't have this at the moment.
Not that it's all that important in the current case, but it might be
handy to have, perhaps, in other version dependent code.

>         Stefan


> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
> index 957a0b8a7c5..05da61dbb2f 100644
> --- a/lisp/progmodes/cc-mode.el
> +++ b/lisp/progmodes/cc-mode.el
> @@ -2565,18 +2565,18 @@
>  ;; Emacs < 22 and XEmacs
>  (defmacro c-advise-fl-for-region (function)
>    (declare (debug t))
> +  (unless (boundp 'font-lock-extend-after-change-region-function)
>    `(defadvice ,function (before get-awk-region activate)
>       ;; Make sure that any string/regexp is completely font-locked.
>       (when c-buffer-is-cc-mode
>         (save-excursion
>  	 (ad-set-arg 1 c-new-END)   ; end
> -	 (ad-set-arg 0 c-new-BEG)))))	; beg
> +	   (ad-set-arg 0 c-new-BEG)))))) ; beg
 
> -(unless (boundp 'font-lock-extend-after-change-region-function)
> -  (c-advise-fl-for-region font-lock-after-change-function)
> -  (c-advise-fl-for-region jit-lock-after-change)
> -  (c-advise-fl-for-region lazy-lock-defer-rest-after-change)
> -  (c-advise-fl-for-region lazy-lock-defer-line-after-change))
> +(c-advise-fl-for-region font-lock-after-change-function)
> +(c-advise-fl-for-region jit-lock-after-change)
> +(c-advise-fl-for-region lazy-lock-defer-rest-after-change)
> +(c-advise-fl-for-region lazy-lock-defer-line-after-change)
 
>  ;; Connect up to `electric-indent-mode' (Emacs 24.4 and later).
>  (defun c-electric-indent-mode-hook ()

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Last use of defadvice in Emacs

[Stefan Monnier]

> I want more than to "perform the test" at compile time.  I want a Lisp
> form that will check whether that variable is bound, and if so, not even
> compile the sub-form.  Something like C's #ifndef preprocessor form.  It
> would look something like
>
>     (hash-if (not
>               (boundp 'font-lock-extend-after-change-region-function))
>              (progn
> 	      (defmacro c-advise-fl-for-region (function) .....)
>               (c-advise-fl-for-region ....)
> 	      ....
> 	      ))
>
> ..  Here the progn form would be neither evaluated nor compiled if that
> font-lock-... variable were boundp.  We don't have this at the moment.
> Not that it's all that important in the current case, but it might be
> handy to have, perhaps, in other version dependent code.

The patch I send does obey the requirement that the `defadvice` will be
"neither evaluated nor compiled" if the variable exists at compile-time.

It is not a separate "hash-if", OTOH.  We can define such a "hash-if",
as seen for example in the `url-http-ntlm` GNU ELPA package:

    (defmacro url-http-ntlm--if-when-compile (cond &rest body)
      (declare (debug t) (indent 1))
      (when (eval cond)
        `(progn ,@body)))

    ;; Remove authorization after redirect.
    (url-http-ntlm--if-when-compile
        (and (boundp 'emacs-major-version)
    	 (< emacs-major-version 25))
      ...
      ... Various code, including, incidentally, a `defadvice` ...
      ...)

Along similar lines, there's AUCTeX's `TeX--if-macro-fboundp`:

    (defmacro TeX--if-macro-fboundp (name then &rest else)
      "Execute THEN if macro NAME is bound and ELSE otherwise.
    Essentially,
    
      (TeX--if-macro-fboundp name then else...)
    
    is equivalent to
    
      (if (fboundp 'name) then else...)
    
    but takes care of byte-compilation issues where the byte-code for
    the latter could signal an error if it has been compiled with
    emacs 24.1 and is then later run by emacs 24.5."
      (declare (indent 2) (debug (symbolp form &rest form)))
      (if (fboundp name)             ;If macro exists at compile-time, just use it.
          then
        `(if (fboundp ',name)               ;Else, check if it exists at run-time.
             (eval ',then)                  ;If it does, then run the then code.
           ,@else)))

No such macro has reached ELisp's core yet, probably because the precise
requirements tend to be subtly different (often depending on the
authors's own preferences about what they want to consider as legitimate
or important use-cases, as is the case in the cc-mode code: what should
happen when compiled on Emacs-NN but run on Emacs-MM?).


        Stefan

Re: Last use of defadvice in Emacs

[Alan Mackenzie]

Hello, Stefan.

On Thu, Apr 07, 2022 at 14:37:14 -0400, Stefan Monnier wrote:
> > I want more than to "perform the test" at compile time.  I want a Lisp
> > form that will check whether that variable is bound, and if so, not even
> > compile the sub-form.  Something like C's #ifndef preprocessor form.  It
> > would look something like

> >     (hash-if (not
> >               (boundp 'font-lock-extend-after-change-region-function))
> >              (progn
> > 	      (defmacro c-advise-fl-for-region (function) .....)
> >               (c-advise-fl-for-region ....)
> > 	      ....
> > 	      ))

> > ..  Here the progn form would be neither evaluated nor compiled if that
> > font-lock-... variable were boundp.  We don't have this at the moment.
> > Not that it's all that important in the current case, but it might be
> > handy to have, perhaps, in other version dependent code.

> The patch I send does obey the requirement that the `defadvice` will be
> "neither evaluated nor compiled" if the variable exists at compile-time.

That's different from, and less demanding than, the "challenge" I set, I
think.  It's not at all important to Emacs's health, but .....

> It is not a separate "hash-if", OTOH.  We can define such a "hash-if",
> as seen for example in the `url-http-ntlm` GNU ELPA package:

>     (defmacro url-http-ntlm--if-when-compile (cond &rest body)
>       (declare (debug t) (indent 1))
>       (when (eval cond)
>         `(progn ,@body)))

>     ;; Remove authorization after redirect.
>     (url-http-ntlm--if-when-compile
>         (and (boundp 'emacs-major-version)
>     	 (< emacs-major-version 25))
>       ...
>       ... Various code, including, incidentally, a `defadvice` ...
>       ...)

Here, a piece of `byte-code' gets compiled for the
url-http-ntlm--if-when-compile call.  This is useless code without any
useful function.  (I've just tried it.)

The action I asked for (or, at least, meant to ask for) was for _no_ code
to get compiled when a condition was not met.  I still believe this is
not possible at the moment.  But the C preprocessor can do it with
#ifndef.

One use for this could be writing unit tests in the same file.el as what
they're testing.  They would get compiled only for a test run.

> Along similar lines, there's AUCTeX's `TeX--if-macro-fboundp`:

>     (defmacro TeX--if-macro-fboundp (name then &rest else)
>       "Execute THEN if macro NAME is bound and ELSE otherwise.
>     Essentially,
    
>       (TeX--if-macro-fboundp name then else...)
    
>     is equivalent to
    
>       (if (fboundp 'name) then else...)
    
>     but takes care of byte-compilation issues where the byte-code for
>     the latter could signal an error if it has been compiled with
>     emacs 24.1 and is then later run by emacs 24.5."
>       (declare (indent 2) (debug (symbolp form &rest form)))
>       (if (fboundp name)             ;If macro exists at compile-time, just use it.
>           then
>         `(if (fboundp ',name)               ;Else, check if it exists at run-time.
>              (eval ',then)                  ;If it does, then run the then code.
>            ,@else)))

I haven't been able to find AUCTeX's source code yet, but I'm not sure
this macro would meet the "no code at all" criterion, either.

> No such macro has reached ELisp's core yet, probably because the
> precise requirements tend to be subtly different (often depending on
> the authors's own preferences about what they want to consider as
> legitimate or important use-cases, as is the case in the cc-mode code:
> what should happen when compiled on Emacs-NN but run on Emacs-MM?).

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Last use of defadvice in Emacs

[Stefan Monnier via CC-Mode-help]

>>     (defmacro url-http-ntlm--if-when-compile (cond &rest body)
>>       (declare (debug t) (indent 1))
>>       (when (eval cond)
>>         `(progn ,@body)))
>
>>     ;; Remove authorization after redirect.
>>     (url-http-ntlm--if-when-compile
>>         (and (boundp 'emacs-major-version)
>>     	 (< emacs-major-version 25))
>>       ...
>>       ... Various code, including, incidentally, a `defadvice` ...
>>       ...)
>
> Here, a piece of `byte-code' gets compiled for the
> url-http-ntlm--if-when-compile call.  This is useless code without any
> useful function.  (I've just tried it.)

That bytecode implements your #if feature.  So if we care about that
feature, we'd remove the `url-http-ntlm--` prefix and move it to
lisp/subr.el, indeed.

I agree that Emacs does not come with the equivalent of #if but the
above macro shows that it can be implemented quite easily if we care to
provide it.

> I haven't been able to find AUCTeX's source code yet, but I'm not sure
> this macro would meet the "no code at all" criterion, either.

Again it depends if you count the code of the macro itself (which
currently lives in AUCTeX but is fundamentally generic so could be
moved to `subr.el`) rather than
only the code resulting from the macroexpansion.


        Stefan

Re: Last use of defadvice in Emacs

[Alan Mackenzie]

Hello, Stefan.

On Fri, Apr 08, 2022 at 13:39:58 -0400, Stefan Monnier wrote:
> >>     (defmacro url-http-ntlm--if-when-compile (cond &rest body)
> >>       (declare (debug t) (indent 1))
> >>       (when (eval cond)
> >>         `(progn ,@body)))

> >>     ;; Remove authorization after redirect.
> >>     (url-http-ntlm--if-when-compile
> >>         (and (boundp 'emacs-major-version)
> >>     	 (< emacs-major-version 25))
> >>       ...
> >>       ... Various code, including, incidentally, a `defadvice` ...
> >>       ...)

> > Here, a piece of `byte-code' gets compiled for the
> > url-http-ntlm--if-when-compile call.  This is useless code without any
> > useful function.  (I've just tried it.)

> That bytecode implements your #if feature.  So if we care about that
> feature, we'd remove the `url-http-ntlm--` prefix and move it to
> lisp/subr.el, indeed.

Apologies, you are right.  That piece of byte code is indeed part of the
macro's definition, not something generated by it.  I tried compiling a
source file with just (require ...) (defun foo ...)
(url-http-ntlm--if-when-compile ..) and (defun bar ...), and in the
..elc, the foo and the bar had nothing intervening between them.

> I agree that Emacs does not come with the equivalent of #if but the
> above macro shows that it can be implemented quite easily if we care to
> provide it.

Yes.  :-)

[ .... ]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).