all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
@ 2022-10-26 17:02 Matt Armstrong
  2022-11-11 13:16 ` Stefan Kangas
  0 siblings, 1 reply; 8+ messages in thread
From: Matt Armstrong @ 2022-10-26 17:02 UTC (permalink / raw)
  To: 58801

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

Tags: patch

(rationale in the patch)

In GNU Emacs 29.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version
 3.24.34, cairo version 1.16.0) of 2022-10-25 built on naz
Repository revision: 9d7ba2b1998afc3664c37d9d1b6f6ca2d68356e9
Repository branch: feature/noverlay
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure 'CFLAGS=-Og -g3' 'CXXFLAGS=-Og -g3' --enable-checking=yes
 --enable-check-lisp-object-type --with-pgtk'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Autoload-the-calc-eval-error-variable.patch --]
[-- Type: text/patch, Size: 1029 bytes --]

From 526d0b31e0d836e7a3c21d831849b8c50da2420e Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Wed, 26 Oct 2022 09:46:37 -0700
Subject: [PATCH] Autoload the `calc-eval-error' variable

* lisp/calc/calc-aent.el: Autoload the `calc-eval-error' variable,
because it is documented as a lisp level option of the `calc-eval'
function, which is also autoloaded.  Otherwise, even (require 'calc)
is not enough to get the variable defined; `calc-eval' must actually
be evaluated.  This squashes byte compiler warnings in code using the
variable.
---
 lisp/calc/calc-aent.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/calc/calc-aent.el b/lisp/calc/calc-aent.el
index ef3e0d4b67..59692beff7 100644
--- a/lisp/calc/calc-aent.el
+++ b/lisp/calc/calc-aent.el
@@ -252,6 +252,7 @@ calc-do-calc-eval
 			     res (cdr res)))
 		     buf)))))))))
 
+;;;###autoload
 (defvar calc-eval-error nil
   "Determines how calc handles errors.
 If nil, return a list containing the character position of error.
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2022-10-26 17:02 bug#58801: [PATCH] Autoload the `calc-eval-error' variable Matt Armstrong
@ 2022-11-11 13:16 ` Stefan Kangas
  2022-11-15 18:24   ` Matt Armstrong
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Kangas @ 2022-11-11 13:16 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 58801

Matt Armstrong <matt@rfc20.org> writes:

> From 526d0b31e0d836e7a3c21d831849b8c50da2420e Mon Sep 17 00:00:00 2001
> From: Matt Armstrong <matt@rfc20.org>
> Date: Wed, 26 Oct 2022 09:46:37 -0700
> Subject: [PATCH] Autoload the `calc-eval-error' variable
>
> * lisp/calc/calc-aent.el: Autoload the `calc-eval-error' variable,
> because it is documented as a lisp level option of the `calc-eval'
> function, which is also autoloaded.  Otherwise, even (require 'calc)
> is not enough to get the variable defined; `calc-eval' must actually
> be evaluated.  This squashes byte compiler warnings in code using the
> variable.

I don't necessarily object strongly or anything, but should we really
autoload a variable just to squash byte compiler warnings?

I think the usual way to do that is to say

    (defvar calc-eval-error)

in the calling code.

> ---
>  lisp/calc/calc-aent.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/calc/calc-aent.el b/lisp/calc/calc-aent.el
> index ef3e0d4b67..59692beff7 100644
> --- a/lisp/calc/calc-aent.el
> +++ b/lisp/calc/calc-aent.el
> @@ -252,6 +252,7 @@ calc-do-calc-eval
>  			     res (cdr res)))
>  		     buf)))))))))
>
> +;;;###autoload
>  (defvar calc-eval-error nil
>    "Determines how calc handles errors.
>  If nil, return a list containing the character position of error.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2022-11-11 13:16 ` Stefan Kangas
@ 2022-11-15 18:24   ` Matt Armstrong
  2022-11-15 18:42     ` Eli Zaretskii
  2022-11-24 19:50     ` Stefan Kangas
  0 siblings, 2 replies; 8+ messages in thread
From: Matt Armstrong @ 2022-11-15 18:24 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 58801

Stefan Kangas <stefankangas@gmail.com> writes:

> Matt Armstrong <matt@rfc20.org> writes:
>
>> From 526d0b31e0d836e7a3c21d831849b8c50da2420e Mon Sep 17 00:00:00 2001
>> From: Matt Armstrong <matt@rfc20.org>
>> Date: Wed, 26 Oct 2022 09:46:37 -0700
>> Subject: [PATCH] Autoload the `calc-eval-error' variable
>>
>> * lisp/calc/calc-aent.el: Autoload the `calc-eval-error' variable,
>> because it is documented as a lisp level option of the `calc-eval'
>> function, which is also autoloaded.  Otherwise, even (require 'calc)
>> is not enough to get the variable defined; `calc-eval' must actually
>> be evaluated.  This squashes byte compiler warnings in code using the
>> variable.
>
> I don't necessarily object strongly or anything, but should we really
> autoload a variable just to squash byte compiler warnings?

Perhaps I can learn something here.  Why refrain from autoloading the
variable in this situation?

Note that in my case I had (require 'calc) in the file that used the
`calc-eval-error' symbol.  The info docs for calc state that (require
'calc) loads nearly everything you need from calc.  I may not understand
something about the design constraints here, but it seems strange to
refrain from autoloading this symbol, since (require 'calc) already
(auto)loads a *lot* of stuff.

> I think the usual way to do that is to say
>
>     (defvar calc-eval-error)
>
> in the calling code.

I think "in the calling code" applies to specific situations.  For
example:

 - A defvar for something x- in package x.

 - Symbols provided by packages that are conditionally loaded, so the
   current package can not rely on (require 'x) to providing `x-'
   symbols at bytcomp time.

 - Situations where the package has inadequate/incorrect autoloads, so
   (require 'x) doesn't provide enough.  I.e. to work around bugs.  ;-)


My first impression is that adding `defvar' to squash bytecomp warnings
for symbols in other packages is the wrong default action, and that the
best idea is for

  (require 'foo)

to provide all symbols 'foo-' that one might need when using the `foo'
package in the normal way.

Notice that (info "(elisp) Converting to Lexical Binding") has this
phrasing:

> A warning about a reference or an assignment to a free variable is
> usually a clear sign that that variable should be marked as
> dynamically scoped, so you need to add an appropriate ‘defvar’ before
> the first use of that variable.

It doesn't state what an "appropriate 'defvar'" is.  Certainly, if the
var is part of the current package, adding a 'defvar' in the same file
makes sense.  If the var is part of some other package, properly
required by the current package, I think that other package is missing
an autoload.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2022-11-15 18:24   ` Matt Armstrong
@ 2022-11-15 18:42     ` Eli Zaretskii
  2022-11-24 19:50     ` Stefan Kangas
  1 sibling, 0 replies; 8+ messages in thread
From: Eli Zaretskii @ 2022-11-15 18:42 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 58801, stefankangas

> Cc: 58801@debbugs.gnu.org
> From: Matt Armstrong <matt@rfc20.org>
> Date: Tue, 15 Nov 2022 10:24:00 -0800
> 
> My first impression is that adding `defvar' to squash bytecomp warnings
> for symbols in other packages is the wrong default action

No, it's the standard solution for byte-compilation warnings.  There
are a few others, all of them better than an actual require.

> and that the best idea is for
> 
>   (require 'foo)
> 
> to provide all symbols 'foo-' that one might need when using the `foo'
> package in the normal way.

That actually loads the package foo, for no good reason.  We don't
necessarily want to use foo, we just want to compile a reference to
its variable or function.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2022-11-15 18:24   ` Matt Armstrong
  2022-11-15 18:42     ` Eli Zaretskii
@ 2022-11-24 19:50     ` Stefan Kangas
  2022-11-26 16:58       ` Matt Armstrong
  1 sibling, 1 reply; 8+ messages in thread
From: Stefan Kangas @ 2022-11-24 19:50 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 58801

Matt Armstrong <matt@rfc20.org> writes:

> Note that in my case I had (require 'calc) in the file that used the
> `calc-eval-error' symbol.  The info docs for calc state that (require
> 'calc) loads nearly everything you need from calc.  I may not understand
> something about the design constraints here, but it seems strange to
> refrain from autoloading this symbol, since (require 'calc) already
> (auto)loads a *lot* of stuff.

So you are saying that if you have a file foo.el, that requires calc,
and then tries to use calc-eval-error variable (documented as part of
the external API), you get a byte-compiler warning?

I agree that this doesn't sound very intuitive.

> My first impression is that adding `defvar' to squash bytecomp warnings
> for symbols in other packages is the wrong default action, and that the
> best idea is for
>
>   (require 'foo)
>
> to provide all symbols 'foo-' that one might need when using the `foo'
> package in the normal way.

So I think we could install your patch.





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2022-11-24 19:50     ` Stefan Kangas
@ 2022-11-26 16:58       ` Matt Armstrong
  2023-09-07  7:51         ` Stefan Kangas
  0 siblings, 1 reply; 8+ messages in thread
From: Matt Armstrong @ 2022-11-26 16:58 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: 58801

Stefan Kangas <stefankangas@gmail.com> writes:

> Matt Armstrong <matt@rfc20.org> writes:
>
>> Note that in my case I had (require 'calc) in the file that used the
>> `calc-eval-error' symbol.  The info docs for calc state that (require
>> 'calc) loads nearly everything you need from calc.  I may not understand
>> something about the design constraints here, but it seems strange to
>> refrain from autoloading this symbol, since (require 'calc) already
>> (auto)loads a *lot* of stuff.
>
> So you are saying that if you have a file foo.el, that requires calc,
> and then tries to use calc-eval-error variable (documented as part of
> the external API), you get a byte-compiler warning?
>
> I agree that this doesn't sound very intuitive.

I regret typing about `require' at all, as my line of argument is
simpler than that.

Running "emacs -Q" comes with `calc-eval' autoloaded.  Since calc
documentation mentions `calc-eval-error' as a configuration variable for
the `calc-eval' behavior, it is makes most sense to autoload either
neither of them or both of them.

(In the particular case of the Calc package, dozens of functions and
variables are already autoloaded.  The omission of `calc-eval-error'
also seems more an oversight than intentional.)


> So I think we could install your patch.

Me too.  ;-)





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2022-11-26 16:58       ` Matt Armstrong
@ 2023-09-07  7:51         ` Stefan Kangas
  2023-11-05 16:38           ` Stefan Kangas
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Kangas @ 2023-09-07  7:51 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 58801

Matt Armstrong <matt@rfc20.org> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Matt Armstrong <matt@rfc20.org> writes:
>>
>>> Note that in my case I had (require 'calc) in the file that used the
>>> `calc-eval-error' symbol.  The info docs for calc state that (require
>>> 'calc) loads nearly everything you need from calc.  I may not understand
>>> something about the design constraints here, but it seems strange to
>>> refrain from autoloading this symbol, since (require 'calc) already
>>> (auto)loads a *lot* of stuff.
>>
>> So you are saying that if you have a file foo.el, that requires calc,
>> and then tries to use calc-eval-error variable (documented as part of
>> the external API), you get a byte-compiler warning?
>>
>> I agree that this doesn't sound very intuitive.
>
> I regret typing about `require' at all, as my line of argument is
> simpler than that.
>
> Running "emacs -Q" comes with `calc-eval' autoloaded.  Since calc
> documentation mentions `calc-eval-error' as a configuration variable for
> the `calc-eval' behavior, it is makes most sense to autoload either
> neither of them or both of them.

Thanks, now I understand the situation better.

We typically avoid autoloading variables, and I'm not sure it's
justified here.  See (info "(elisp) When to Autoload") for details.

Could we instead just declare it in calc.el?  I believe that should
silence any warnings from the byte-compiler.  It's a one line change:

    (defvar calc-eval-error)

Or will that not work in your use case for some reason?

> (In the particular case of the Calc package, dozens of functions and
> variables are already autoloaded.  The omission of `calc-eval-error'
> also seems more an oversight than intentional.)

FWIW, I couldn't find any autoloaded variables in calc-loaddefs.el.
What am I missing?





^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#58801: [PATCH] Autoload the `calc-eval-error' variable
  2023-09-07  7:51         ` Stefan Kangas
@ 2023-11-05 16:38           ` Stefan Kangas
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Kangas @ 2023-11-05 16:38 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 58801-done

Version: 30.1

Stefan Kangas <stefankangas@gmail.com> writes:

> We typically avoid autoloading variables, and I'm not sure it's
> justified here.  See (info "(elisp) When to Autoload") for details.
>
> Could we instead just declare it in calc.el?  I believe that should
> silence any warnings from the byte-compiler.  It's a one line change:
>
>     (defvar calc-eval-error)
>
> Or will that not work in your use case for some reason?
>
>> (In the particular case of the Calc package, dozens of functions and
>> variables are already autoloaded.  The omission of `calc-eval-error'
>> also seems more an oversight than intentional.)
>
> FWIW, I couldn't find any autoloaded variables in calc-loaddefs.el.
> What am I missing?

No further comments here within 2 months, so I've pushed the above
proposed fix to master, and I'm closing this bug.

If this is still an issue, please reply to this email (use "Reply to
all" in your email client) and we can reopen the bug report.

[1: ad82bc9b29e]: 2023-11-05 17:36:21 +0100
  Declare calc-eval-error in calc.el
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ad82bc9b29eacad29a441bbb4e87bd09ef1ff1c4





^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-11-05 16:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-26 17:02 bug#58801: [PATCH] Autoload the `calc-eval-error' variable Matt Armstrong
2022-11-11 13:16 ` Stefan Kangas
2022-11-15 18:24   ` Matt Armstrong
2022-11-15 18:42     ` Eli Zaretskii
2022-11-24 19:50     ` Stefan Kangas
2022-11-26 16:58       ` Matt Armstrong
2023-09-07  7:51         ` Stefan Kangas
2023-11-05 16:38           ` Stefan Kangas

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.