Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

>     * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

Why?


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>     * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.
>
> Why?

Used in menu-bar.el.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Glenn Morris]

Michael Albinus wrote:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>>>     * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.
>>
>> Why?
>
> Used in menu-bar.el.

But not in any significant way?


--- i/lisp/display-line-numbers.el
+++ w/lisp/display-line-numbers.el
@@ -38,7 +38,6 @@ display-line-numbers
   "Display line numbers in the buffer."
   :group 'display)
 
-;;;###autoload
 (defcustom display-line-numbers-type t
   "The default type of line numbers to use in `display-line-numbers-mode'.
 See `display-line-numbers' for value options."
diff --git i/lisp/menu-bar.el w/lisp/menu-bar.el
index 05a336b..e02217f 100644
--- i/lisp/menu-bar.el
+++ w/lisp/menu-bar.el
@@ -1101,6 +1101,9 @@ menu-bar-showhide-tool-bar-menu-customize-enable-bottom
                     :button (:radio . (eq tool-bar-mode nil))))
       menu)))
 
+;; Following function calls will autoload the library.
+(defvar display-line-numbers-type)
+
 (defun menu-bar-display-line-numbers-mode (type)
   (setq display-line-numbers-type type)
   (if global-display-line-numbers-mode



This message brought to you by SAUNA: The Society Against UNnecessary Autoloads

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Mon, 24 Jul 2017 12:57:41 -0400
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>, emacs-devel@gnu.org
> 
> --- i/lisp/display-line-numbers.el
> +++ w/lisp/display-line-numbers.el
> @@ -38,7 +38,6 @@ display-line-numbers
>    "Display line numbers in the buffer."
>    :group 'display)
>  
> -;;;###autoload
>  (defcustom display-line-numbers-type t
>    "The default type of line numbers to use in `display-line-numbers-mode'.
>  See `display-line-numbers' for value options."
> diff --git i/lisp/menu-bar.el w/lisp/menu-bar.el
> index 05a336b..e02217f 100644
> --- i/lisp/menu-bar.el
> +++ w/lisp/menu-bar.el
> @@ -1101,6 +1101,9 @@ menu-bar-showhide-tool-bar-menu-customize-enable-bottom
>                      :button (:radio . (eq tool-bar-mode nil))))
>        menu)))
>  
> +;; Following function calls will autoload the library.
> +(defvar display-line-numbers-type)
> +
>  (defun menu-bar-display-line-numbers-mode (type)
>    (setq display-line-numbers-type type)
>    (if global-display-line-numbers-mode

How is this better than the autoload?  At least the autoload doesn't
need a comment to explain why it is there.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

>> +;; Following function calls will autoload the library.
>> +(defvar display-line-numbers-type)
>> +
>> (defun menu-bar-display-line-numbers-mode (type)
>> (setq display-line-numbers-type type)
>> (if global-display-line-numbers-mode
> How is this better than the autoload?

Autoloading variables is a source of corner-case problems, and
autoloading defcustoms even more so.  It's not serious enough to ban
such autoloads, but it's a good practice to try and avoid them when it
can be done cheaply, and to clearly document them when we do need them.

> At least the autoload doesn't need a comment to explain why it
> is there.

I find this very amusing:
- my asking Micheal "Why?" is another way to say "I'd have liked to see
  a comment explaining why we need the autoload".
- the comment on the defvar isn't needed anyway: it's pretty clear why the
  defvar is used, and even if nothing autoloads the library, the code with
  defvar+setq is correct.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 24 Jul 2017 13:29:39 -0400
> 
> >> +;; Following function calls will autoload the library.
> >> +(defvar display-line-numbers-type)
> >> +
> >> (defun menu-bar-display-line-numbers-mode (type)
> >> (setq display-line-numbers-type type)
> >> (if global-display-line-numbers-mode
> > How is this better than the autoload?
> 
> Autoloading variables is a source of corner-case problems, and
> autoloading defcustoms even more so.

Emacs is full of corner-case problems, one more or one less won't
change anything.

And can you PLEASE make the problems documented somewhere?  The way
you are fighting this makes it sound like some black magic that only
you understand.  Maybe if the problem description is out in the open,
it will be easier to get people to follow your views?

> It's not serious enough to ban such autoloads, but it's a good
> practice to try and avoid them when it can be done cheaply, and to
> clearly document them when we do need them.

Good practice comes out from clearly documented problems and well
explained solutions.  We should _educate_ people to avoid such
problems, not haunt them with review comments that, left unexplained,
make little sense.  If it's important, it should be in "code
conventions" in the ELisp manual.

> - the comment on the defvar isn't needed anyway: it's pretty clear why the
>   defvar is used

It isn't clear to me, at least.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

>> - the comment on the defvar isn't needed anyway: it's pretty clear why the
>>   defvar is used
> It isn't clear to me, at least.

How so?  It silences the compiler warning, just like all such (defvar foo).
And that's the only thing it does, because there is nothing more needed
for a `setq'.  It's been that way "for ever".


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Mon, 24 Jul 2017 14:16:42 -0400
> 
> >> - the comment on the defvar isn't needed anyway: it's pretty clear why the
> >>   defvar is used
> > It isn't clear to me, at least.
> 
> How so?

Because just _looking_ at the code doesn't explain why it's there, and
its relation to autoloading is not immediately obvious.

> It silences the compiler warning, just like all such (defvar foo).

But this is not about silencing the compiler, this is about
autoloading.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

>> >> - the comment on the defvar isn't needed anyway: it's pretty clear why the
>> >>   defvar is used
>> > It isn't clear to me, at least.
>> How so?
> Because just _looking_ at the code doesn't explain why it's there, and
> its relation to autoloading is not immediately obvious.

There is no relation to autoloading.
>> It silences the compiler warning, just like all such (defvar foo).
> But this is not about silencing the compiler, this is about
> autoloading.

No, it's really just about silencing the byte-compiler.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

>> Autoloading variables is a source of corner-case problems, and
>> autoloading defcustoms even more so.
> Emacs is full of corner-case problems, one more or one less won't
> change anything.

Of course, as maintainer you may choose to add yet-more corner case problems.
But it does sound like an odd choice when there's a simple and standard
solution that doesn't suffer from those.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 24 Jul 2017 14:20:35 -0400
> 
> >> Autoloading variables is a source of corner-case problems, and
> >> autoloading defcustoms even more so.
> > Emacs is full of corner-case problems, one more or one less won't
> > change anything.
> 
> Of course, as maintainer you may choose to add yet-more corner case problems.
> But it does sound like an odd choice when there's a simple and standard
> solution that doesn't suffer from those.

I'd need first to better understand the problem and its proposed
solutions.  How can you expect me to make up my mind about this
without at least that much?  As of now, I don't understand well enough
the choice I have; without that, Michael's code looks to me as a
perfectly valid solution for a real problems, and I see no reason to
look for an alternative.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

> I'd need first to better understand the problem and its proposed
> solutions.  How can you expect me to make up my mind about this
> without at least that much?  As of now, I don't understand well enough
> the choice I have; without that, Michael's code looks to me as a
> perfectly valid solution for a real problems, and I see no reason to
> look for an alternative.

As far as I know, there was no "real problem".  There was
only a byte-compiler warning.  He chose to silence it with an
`autoload', because he thought the warning was real.
Glenn pointed out that a simple defvar to silence the byte-compiler
was sufficient, because the warning was spurious.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 24 Jul 2017 16:02:29 -0400
> 
> > I'd need first to better understand the problem and its proposed
> > solutions.  How can you expect me to make up my mind about this
> > without at least that much?  As of now, I don't understand well enough
> > the choice I have; without that, Michael's code looks to me as a
> > perfectly valid solution for a real problems, and I see no reason to
> > look for an alternative.
> 
> As far as I know, there was no "real problem".  There was
> only a byte-compiler warning.  He chose to silence it with an
> `autoload', because he thought the warning was real.
> Glenn pointed out that a simple defvar to silence the byte-compiler
> was sufficient, because the warning was spurious.

We are miscommunicating.  By "the problem" I meant the reason(s) why
autoloading a defcustom should be considered undesirable, and by
"proposed solutions" the alternatives for making sure a defcustom'd
variable is defined when it is needed.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

> We are miscommunicating.  By "the problem" I meant the reason(s) why
> autoloading a defcustom should be considered undesirable, and by
> "proposed solutions" the alternatives for making sure a defcustom'd
> variable is defined when it is needed.

There is no need for the variable to be defined in order to `setq' it,
which is why a simple (defvar foo) is sufficient here.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

On July 25, 2017 6:22:32 AM GMT+03:00, Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:
> > We are miscommunicating.  By "the problem" I meant the reason(s) why
> > autoloading a defcustom should be considered undesirable, and by
> > "proposed solutions" the alternatives for making sure a defcustom'd
> > variable is defined when it is needed.
> 
> There is no need for the variable to be defined in order to `setq' it,
> which is why a simple (defvar foo) is sufficient here.
> 
> 
>         Stefan

This is not what I asked about.  I asked why autoloading defcustoms is
"considered harmful".  If it isn't, then there's nothing wrong with the
original code as pushed.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> We are miscommunicating.  By "the problem" I meant the reason(s) why
>> autoloading a defcustom should be considered undesirable, and by
>> "proposed solutions" the alternatives for making sure a defcustom'd
>> variable is defined when it is needed.
>
> There is no need for the variable to be defined in order to `setq' it,
> which is why a simple (defvar foo) is sufficient here.

In menu-bar.el (the case we are speaking about) and every other package,
which might be interested to modify this variable prior calling the
corresponding autoloaded function. I still don't see why it is better,
to mandate every package using that variable a (defvar foo) .

And if this is better, it must be documented in the manual with a reasoning.

>         Stefan

Best regards, Michael.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

>>> We are miscommunicating.  By "the problem" I meant the reason(s) why
>>> autoloading a defcustom should be considered undesirable, and by
>>> "proposed solutions" the alternatives for making sure a defcustom'd
>>> variable is defined when it is needed.
>> There is no need for the variable to be defined in order to `setq' it,
>> which is why a simple (defvar foo) is sufficient here.
> In menu-bar.el (the case we are speaking about) and every other package,
> which might be interested to modify this variable prior calling the
> corresponding autoloaded function. I still don't see why it is better,
> to mandate every package using that variable a (defvar foo).

- There is currently no other package in this situation.
  And I don't expect a deluge of such packages either.
- Another package might do (require 'display-line-numbers) anyway.
- In general when a package doesn't want to (require 'bar) but needs
  to let-bind or set some variable bar-baz, the answer is to either live
  with the byte-compiler warning (that's the typical case for code in
  ~/.emacs) or to add a (defvar bar-baz).  It's been that way for ever.

Autoloading the variable is used when you need to know the var's value
(rather than merely set it) and where (require 'bar) is undesirable.

> And if this is better, it must be documented in the manual with a reasoning.

Simple reason: the code worked just fine, so all there was to fix was
a warning, and (defvar bar-baz) is the tool specifically designed to
silence those compiler warnings.

The autoload you added won't cause you to rot in hell the for eternity
(I think), but it's definitely not the best tool to fix the problem at hand.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Glenn Morris]

[Seems I was dropped from the Cc again. Thanks, Gmane.]

Michael Albinus wrote:

> And if this is better, it must be documented in the manual with a reasoning.

Done.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>,  Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 26 Jul 2017 12:54:50 -0400
> 
> > And if this is better, it must be documented in the manual with a reasoning.
> 
> Done.

This part needs some rationale, IMO:

  +@item
  +Don't autoload a user option just so that a user can set it.

Thanks.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Michael Albinus]

Glenn Morris <rgm@gnu.org> writes:

> --- i/lisp/display-line-numbers.el
> +++ w/lisp/display-line-numbers.el
> @@ -38,7 +38,6 @@ display-line-numbers
>    "Display line numbers in the buffer."
>    :group 'display)
>  
> -;;;###autoload
>  (defcustom display-line-numbers-type t
>    "The default type of line numbers to use in `display-line-numbers-mode'.
>  See `display-line-numbers' for value options."
> diff --git i/lisp/menu-bar.el w/lisp/menu-bar.el
> index 05a336b..e02217f 100644
> --- i/lisp/menu-bar.el
> +++ w/lisp/menu-bar.el
> @@ -1101,6 +1101,9 @@ menu-bar-showhide-tool-bar-menu-customize-enable-bottom
>                      :button (:radio . (eq tool-bar-mode nil))))
>        menu)))
>  
> +;; Following function calls will autoload the library.
> +(defvar display-line-numbers-type)
> +
>  (defun menu-bar-display-line-numbers-mode (type)
>    (setq display-line-numbers-type type)
>    (if global-display-line-numbers-mode
>
> This message brought to you by SAUNA: The Society Against UNnecessary Autoloads

If the user prefers another value but the default one,
`display-line-numbers-type' shall be modified prior the first call of
`display-line-numbers-mode' or global-display-line-numbers-mode'.
A typical use case for the autoload cookie.

Best regards, Michael.

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Stefan Monnier]

> If the user prefers another value but the default one,
> `display-line-numbers-type' shall be modified prior the first call of
> `display-line-numbers-mode' or global-display-line-numbers-mode'.
> A typical use case for the autoload cookie.

You can setq a variable long before it's defined.
So no, this is not a typical case for an autoload.


        Stefan

Re: [Emacs-diffs] master 012487b: * lisp/display-line-numbers.el (display-line-numbers-type): Autoload it.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> You can setq a variable long before it's defined.

Plus compiler warnings.

> So no, this is not a typical case for an autoload.

What's so bad with the autoload cookie, that it needs SAUNA?

>         Stefan

Best regards, Michael.