bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Drew Adams]

It seems that whenever users ask a function-advising question anywhere,
Stefan lets them know that `defadvice' is nearly deprecated and not
really being bug-fixed or developed further.  He advises ;-) them to use
the `nadvice.el' features (e.g., `add-function') instead.

Wunderbar.  But meanwhile, there is nothing in the manual about any of
the new stuff.  Still, users are being discouraged from using `defadvice'.
This is not right.  The new advice functionality should be fully
documented, and that should be done before advising people here and
there to use the new and abandon the old.

This bug report is to ask for full doc of `nadvice.el' features in the
Elisp manual in the Emacs 24.4 release.

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2014-01-01 on ODIEONE
Bzr revision: 115827 eggert@cs.ucla.edu-20140101192741-bi5hb4xb4kdi2zpw
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 'CFLAGS=-O0 -g3' LDFLAGS=-Lc:/Devel/emacs/lib
 CPPFLAGS=-Ic:/Devel/emacs/include'

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Glenn Morris]

Drew Adams wrote:

> This bug report is to ask for full doc of `nadvice.el' features in the
> Elisp manual in the Emacs 24.4 release.

Bug reports like this are unnecessary (unless you'd like to supply a patch).
Items in NEWS without +++ or --- markup are reviewed for the necessary
doc changes before release. As previously explained to you in eg

http://debbugs.gnu.org/14801#16
http://debbugs.gnu.org/12346#15

and I'm pretty sure several other times.

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Stefan Monnier]

> This bug report is to ask for full doc of `nadvice.el' features in the
> Elisp manual in the Emacs 24.4 release.

We're still waiting for your patch,


        Stefan

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Andy Moreton]

On Thu 09 Jan 2014, Stefan Monnier wrote:

>> This bug report is to ask for full doc of `nadvice.el' features in the
>> Elisp manual in the Emacs 24.4 release.
>
> We're still waiting for your patch,
>
>
>         Stefan

As the author of nadvice.el, I would have thought that it was incumbent
upon you to provide adequate documentation. A rationale to explain why
the existing advice package needs changing would also be helpful.

As an aside, storing raw bytecode in advice--where-alist seems hackish.
Is it not possible to construct this byte code from something more
human readable at compile time ?

    AndyM

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Leo Liu]

On 2014-01-10 10:29 +0800, Andy Moreton wrote:
> As the author of nadvice.el, I would have thought that it was incumbent
> upon you to provide adequate documentation. A rationale to explain why
> the existing advice package needs changing would also be helpful.

In Stefan's defence this is him saying help me out. I am sure as the
maintainer of emacs he has plenty in his plate that some lower-priority
tasks may have to fall in others hands ;)

Leo

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Daniel Colascione]

On 01/09/2014 07:28 PM, Leo Liu wrote:
> On 2014-01-10 10:29 +0800, Andy Moreton wrote:
>> As the author of nadvice.el, I would have thought that it was incumbent
>> upon you to provide adequate documentation. A rationale to explain why
>> the existing advice package needs changing would also be helpful.
>
> In Stefan's defence this is him saying help me out. I am sure as the
> maintainer of emacs he has plenty in his plate that some lower-priority
> tasks may have to fall in others hands ;)

It'd be easier to document nadvice if there were a clear reason for it 
to exist. Regular advice works fine and has for a long time. It's very 
well documented has many useful features. It integrates with help. It 
supports various interactive commands. I have no plans to use nadvice 
myself any time soon and oppose deprecating regular advice.

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Drew Adams]

> >> As the author of nadvice.el, I would have thought that it was
> >> incumbent upon you to provide adequate documentation. A rationale
> >> to explain why the existing advice package needs changing would
> >> also be helpful.
> >
> > In Stefan's defence this is him saying help me out. I am sure as
> > the maintainer of emacs he has plenty in his plate that some
> > lower-priority tasks may have to fall in others hands ;)
> 
> It'd be easier to document nadvice if there were a clear reason for
> it to exist. Regular advice works fine and has for a long time. It's
> very well documented has many useful features. It integrates with
> help.  It supports various interactive commands. I have no plans
> to use nadvice myself any time soon and oppose deprecating regular
> advice.

1. I suspect that there is some misunderstanding here.  Let me try
to clear some of it up, if so.

I filed the bug report, because Stefan often suggests to people to
use the new advice system provided by nadvice.el.  He tells them
that the "old", `defadvice' system is on the way out (will be
deprecated at some point).  I noticed that currently the only
advice system documented in the Elisp manual is the "old" one,
and there is no mention of it becoming deprecated, and there is
no mention of any other (e.g. nadvice.el) advice system.
The purpose of the bug was to get the new policy reflected in the
Elisp manual.

Glenn closed the bug, reminding me that this new feature is
flagged in the NEWS file as something that needs to be documented
before the Emacs 24.4 release.  IOW, the bug report was not needed
(so it is noise) - the feature is anyway slated to be documented.

Does that help clarify things?  The use of nadvice functionality
does need to be documented in the Elisp manual for 24.4, but they
already plan to do that.  If someone wants to help by submitting
a patch to the manual, they are welcome to do so.  That,
apparently, is Stefan's message here.

2. As far as I know (but someone will correct me if I'm wrong),
Stefan has already decided to deprecate the use of defadvice, like
it or not.  I have filed other bugs for defadvice, and he has made
it clear for at least some that they won't be fixed, saying:
"I have no interest in improving advice.el."

In sum, it seems clear that they do not intend to fix at least
some defadvice problems, since defadvice is anyway on its way out.

Some defadvice bugs I reported recently:

* #14070 - a defadvice doc bug.

* #14130 - another defadvice doc bug.

* #14734 - inability now to update the doc of an advised function
(the new doc is available only by way of a link from the original
doc now).

* #15666 - you can no longer advise a special form.
(You could do that in the past, but that was a while ago - this
regression is unrelated to the move to nadvice.)

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Leo Liu]

On 2014-01-10 11:39 +0800, Daniel Colascione wrote:
> It'd be easier to document nadvice if there were a clear reason for it
> to exist. Regular advice works fine and has for a long time. It's very
> well documented has many useful features. It integrates with help. It
> supports various interactive commands. I have no plans to use nadvice
> myself any time soon and oppose deprecating regular advice.

I agree. People will have a few dozens of defadvice's in their init
file. I think nadvice might be able to cover 99% of defadvice over time.
The code is at manageable size (a few hindered lines) and looks clean.

But I haven't started using it since I am still mostly using 24.3.

Leo

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Stefan Monnier]

> A rationale to explain why the existing advice package needs changing
> would also be helpful.

Here are some of the reasons:
- I implemented add-function, which is a different feature not provided
  by advice.el.
- implementing advice-add on top of the add-function code takes just 8KB.
- by contrast advice.el was 150KB (now reduced to 130KB by using nadvice.el).
- once add-function is documented, documenting advice-add is similarly
  much simpler than defadvice.
- advice-add fixes several problems in defadvice:
  - the fact that defadvice does not expand macros in its code unless
    you explicitly ask for the advice to be compiled.
  - the fact that defadvice only compiles either too early (via the
    "precompilation", which is rarely used and rightly so) or too late
    (when activating the advice), so you typically won't get any
    byte-compiler warnings.
  - ad-do-it and ad-return-value in `around' advice don't work the way
    people intuitively expect them to.
  - ad-activate and ad-deactivate have global effects (i.e. they affect
    all advices applied to that symbol), so any use of those within
    a package is a bug in the waiting.
  - the distinction between enabling/disabling and
    activating/deactivating is "too subtle" for the users of defadvice:
    it's really just a reflection of the underlying implementation.
  - advice.el is much too large to be preloaded, so for example debug.el
    refrained from using it.
    
> As an aside, storing raw bytecode in advice--where-alist seems hackish.

Agreed.

> Is it not possible to construct this byte code from something more
> human readable at compile time ?

It's possible, but the pre-existing code which does something like that
is in the byte-compiler and does not provide quite the right feature.
It didn't seem worth the trouble to restructure that code just for that
one use.


        Stefan

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Drew Adams]

> > A rationale to explain why the existing advice package needs
> > changing would also be helpful.
> 
> Here are some of the reasons:

Excellent.  Very glad to see design rationale documented like this.
Seriously.  (Please keep it up.)

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Daniel Colascione]

On 01/10/2014 07:15 AM, Drew Adams wrote:
>>> A rationale to explain why the existing advice package needs
>>> changing would also be helpful.
>>
>> Here are some of the reasons:
>
> Excellent.  Very glad to see design rationale documented like this.
> Seriously.  (Please keep it up.)

Agreed. nadvice sounds like a good implementation change. The advice 
*interface*, however, isn't going away, so I'm hesitant to recommend the 
nadvice interface as well. Users will be confused about which to use. 
(Yes, we can tell users that advice is deprecated, but then a very large 
amount of working elisp code users see is "deprecated". What message 
does that send?)

There's one part where I'm not sure I agree though.

 >>  - advice.el is much too large to be preloaded,
 >> so for example debug.el refrained from using it.

So what if it's large? Isn't it *because* a commonly-used package is 
large that we want to preload it? This way, we pay up-front for the cost 
of loading that package instead of making users load it on each start.

bug#16402: 24.3.50; Document nadvice.el stuff in Elisp manual before Emacs 24.4

[Stefan Monnier]

> Agreed.  nadvice sounds like a good implementation change.  The advice
> *interface*, however, isn't going away, so I'm hesitant to recommend the
> nadvice interface as well.

The problems that advice-add fixes as compared to defadvice are either
problems in the interface, or internal problems which were not solved by
retargetting defadvice on top of advice-add.  IOW they're still in
defadvice and are not likely to disappear any time soon.

The advice-add interface is simpler, which is why I advocate it.

> (Yes, we can tell users that advice is deprecated, but then a very
> large amount of working elisp code users see is "deprecated".

Yes, it'll take time for the code to move to advice-add.  There's not
much we can do about it.

I don't think advice.el will really disappear, just like cl.el won't
really disappear.  Maybe at some point it'll move to GNU ELPA, tho.

>>> - advice.el is much too large to be preloaded,
>>> so for example debug.el refrained from using it.
> So what if it's large? Isn't it *because* a commonly-used package is large
> that we want to preload it? This way, we pay up-front for the cost of
> loading that package instead of making users load it on each start.

Could be.  But the fact is that advice.el was not preloaded because it
was perceived to be too big and not used enough to justify it.  And in
many cases the reason why it wasn't used is because it didn't seem worth
the trouble loading this big package just to tweak this little function.


        Stefan