* Warnings about keymaps @ 2009-09-11 9:04 Eli Zaretskii 2009-09-11 17:34 ` Glenn Morris 2009-09-11 17:43 ` Stefan Monnier 0 siblings, 2 replies; 13+ messages in thread From: Eli Zaretskii @ 2009-09-11 9:04 UTC (permalink / raw) To: emacs-devel Compilation of Lisp files yields warnings such as these two: Compiling emacs/lisp/cvs-status.el In toplevel form: emacs/lisp/cvs-status.el:93:22:Warning: variable assignment to constant `cvs-status-mode-map' Compiling emacs/lisp/diff-mode.el In toplevel form: emacs/lisp/diff-mode.el:1273:70:Warning: variable assignment to constant `diff-mode-map' Sounds like a bug to me. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-11 9:04 Warnings about keymaps Eli Zaretskii @ 2009-09-11 17:34 ` Glenn Morris 2009-09-11 17:43 ` Stefan Monnier 1 sibling, 0 replies; 13+ messages in thread From: Glenn Morris @ 2009-09-11 17:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel Eli Zaretskii wrote: > emacs/lisp/cvs-status.el:93:22:Warning: variable assignment to constant > `cvs-status-mode-map' I think this is because easy-mmode-defmap defines cvs-status-mode-map as a constant. Then, later on, the define-derived-mode macro tries to do a defvar on cvs-status-mode-map. The simple change is to make easy-mmode-defmap use defvar instead of defconst; the slightly more correct thing (that I could not get to work...) is to make define-derived-mode only defvar the mode map if it is not already defined. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-11 9:04 Warnings about keymaps Eli Zaretskii 2009-09-11 17:34 ` Glenn Morris @ 2009-09-11 17:43 ` Stefan Monnier 2009-09-14 19:10 ` Glenn Morris 1 sibling, 1 reply; 13+ messages in thread From: Stefan Monnier @ 2009-09-11 17:43 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel > Compilation of Lisp files yields warnings such as these two: > Compiling emacs/lisp/cvs-status.el > In toplevel form: > emacs/lisp/cvs-status.el:93:22:Warning: variable assignment to constant > `cvs-status-mode-map' > Compiling emacs/lisp/diff-mode.el > In toplevel form: > emacs/lisp/diff-mode.el:1273:70:Warning: variable assignment to > constant `diff-mode-map' > Sounds like a bug to me. Yes, there's some kind of bug in there. The bug can be seen more directly by byte-compiling the file below: (defconst foo nil) (defvar foo nil) Looks like the "assignment-to-constant" warning should be more careful to distinguish defvar from setq. Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-11 17:43 ` Stefan Monnier @ 2009-09-14 19:10 ` Glenn Morris 2009-09-14 21:46 ` Stefan Monnier 0 siblings, 1 reply; 13+ messages in thread From: Glenn Morris @ 2009-09-14 19:10 UTC (permalink / raw) To: Stefan Monnier; +Cc: Eli Zaretskii, emacs-devel Stefan Monnier wrote: > The bug can be seen more directly by byte-compiling the file below: > > (defconst foo nil) > (defvar foo nil) > > Looks like the "assignment-to-constant" warning should be more > careful to distinguish defvar from setq. This seems a fairly nonsensical thing to do, so why shouldn't the compiler warn about it? The defvar is a best a no-op, at worst a bug (if you were expecting it to have an effect). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-14 19:10 ` Glenn Morris @ 2009-09-14 21:46 ` Stefan Monnier 2009-09-15 2:45 ` Glenn Morris 2009-09-15 15:13 ` Richard Stallman 0 siblings, 2 replies; 13+ messages in thread From: Stefan Monnier @ 2009-09-14 21:46 UTC (permalink / raw) To: Glenn Morris; +Cc: Eli Zaretskii, emacs-devel >> The bug can be seen more directly by byte-compiling the file below: >> >> (defconst foo nil) >> (defvar foo nil) >> >> Looks like the "assignment-to-constant" warning should be more >> careful to distinguish defvar from setq. > This seems a fairly nonsensical thing to do, so why shouldn't the > compiler warn about it? It could, but it shouldn't complain about assignment to a constant, since the defvar will not perform any assignment. I.e. maybe a warning is OK, but not the warning we currently get. > The defvar is a best a no-op, at worst a bug > (if you were expecting it to have an effect). In the OP's case the defvar was expected to have as effect to "provide a default value to the var in case it's not defined elsewhere". So it seems to behave as expected (it's a no-op since the var is defined elsewhere). Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-14 21:46 ` Stefan Monnier @ 2009-09-15 2:45 ` Glenn Morris 2009-09-15 13:40 ` Stefan Monnier 2009-09-15 15:13 ` Richard Stallman 1 sibling, 1 reply; 13+ messages in thread From: Glenn Morris @ 2009-09-15 2:45 UTC (permalink / raw) To: Stefan Monnier; +Cc: Eli Zaretskii, emacs-devel Stefan Monnier wrote: > It could, but it shouldn't complain about assignment to a constant, > since the defvar will not perform any assignment. I.e. maybe a warning > is OK, but not the warning we currently get. How about this? Or perhaps the defvar should just be silently optimized away, like the default-boundp test seems to be trying to do. *** bytecomp.el 10 Sep 2009 06:22:30 -0000 2.258 --- bytecomp.el 14 Sep 2009 23:30:56 -0000 *************** *** 4025,4032 **** (let ((tmp (make-symbol "defconst-tmp-var"))) `(funcall '(lambda (,tmp) (defconst ,var ,tmp)) ,value)) ;; `defvar' sets `var' only when unbound. ! `(if (not (default-boundp ',var)) (setq-default ,var ,value)))) (when (eq fun 'defconst) ;; This will signal an appropriate error at runtime. `(eval ',form))) --- 4036,4046 ---- (let ((tmp (make-symbol "defconst-tmp-var"))) `(funcall '(lambda (,tmp) (defconst ,var ,tmp)) ,value)) + (if (memq var byte-compile-const-variables) + (byte-compile-warn "attempt to defvar constant `%s' \ + has no effect" var) ;; `defvar' sets `var' only when unbound. ! `(if (not (default-boundp ',var)) (setq-default ,var ,value))))) (when (eq fun 'defconst) ;; This will signal an appropriate error at runtime. `(eval ',form))) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-15 2:45 ` Glenn Morris @ 2009-09-15 13:40 ` Stefan Monnier 2009-09-17 7:32 ` Glenn Morris 0 siblings, 1 reply; 13+ messages in thread From: Stefan Monnier @ 2009-09-15 13:40 UTC (permalink / raw) To: Glenn Morris; +Cc: Eli Zaretskii, emacs-devel >> It could, but it shouldn't complain about assignment to a constant, >> since the defvar will not perform any assignment. I.e. maybe a warning >> is OK, but not the warning we currently get. > How about this? Or perhaps the defvar should just be silently > optimized away, like the default-boundp test seems to be trying to do. This warning might be OK, but I have some issues with it: - most importantly: it doesn't solve the problem at hand which is how to get rid of the other warning. - what's its use? It's not like we've seen lots of bugs where people use both defconst and defvar for a variable. - Finally, if the defconst+defvar is really what you want to do (as in the present case where the defconst and the defvar both come from macros that we may not want to change), how do you silence the warning? Stefan > *** bytecomp.el 10 Sep 2009 06:22:30 -0000 2.258 > --- bytecomp.el 14 Sep 2009 23:30:56 -0000 > *************** > *** 4025,4032 **** > (let ((tmp (make-symbol "defconst-tmp-var"))) > `(funcall '(lambda (,tmp) (defconst ,var ,tmp)) > ,value)) > ;; `defvar' sets `var' only when unbound. > ! `(if (not (default-boundp ',var)) (setq-default ,var ,value)))) > (when (eq fun 'defconst) > ;; This will signal an appropriate error at runtime. > `(eval ',form))) > --- 4036,4046 ---- > (let ((tmp (make-symbol "defconst-tmp-var"))) > `(funcall '(lambda (,tmp) (defconst ,var ,tmp)) > ,value)) > + (if (memq var byte-compile-const-variables) > + (byte-compile-warn "attempt to defvar constant `%s' \ > + has no effect" var) > ;; `defvar' sets `var' only when unbound. > ! `(if (not (default-boundp ',var)) (setq-default ,var ,value))))) > (when (eq fun 'defconst) > ;; This will signal an appropriate error at runtime. > `(eval ',form))) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-15 13:40 ` Stefan Monnier @ 2009-09-17 7:32 ` Glenn Morris 2009-09-17 14:02 ` Stefan Monnier 0 siblings, 1 reply; 13+ messages in thread From: Glenn Morris @ 2009-09-17 7:32 UTC (permalink / raw) To: Stefan Monnier; +Cc: Eli Zaretskii, emacs-devel Stefan Monnier wrote: > - most importantly: it doesn't solve the problem at hand which is how > to get rid of the other warning. The other warning? > - what's its use? It's not like we've seen lots of bugs where people > use both defconst and defvar for a variable. It simply replaces the current "variable assignment to a constant" warning with a slightly more informative one, as you suggested. > - Finally, if the defconst+defvar is really what you want to do (as in > the present case where the defconst and the defvar both come from > macros that we may not want to change), how do you silence > the warning? The same way you silence the current one about variable assignment to a constant. :) I don't think is a particularly useful change. I think it would be better to change define-derived-mode to not defvar the map if it is already defined; or to change easy-mode-defmap to not use defconst (are you saying users should not add key bindings to log-edit-mode-map, for example?). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-17 7:32 ` Glenn Morris @ 2009-09-17 14:02 ` Stefan Monnier 2009-09-18 22:25 ` Glenn Morris 0 siblings, 1 reply; 13+ messages in thread From: Stefan Monnier @ 2009-09-17 14:02 UTC (permalink / raw) To: Glenn Morris; +Cc: Eli Zaretskii, emacs-devel >> - most importantly: it doesn't solve the problem at hand which is how >> to get rid of the other warning. > The other warning? >> - what's its use? It's not like we've seen lots of bugs where people >> use both defconst and defvar for a variable. > It simply replaces the current "variable assignment to a constant" > warning with a slightly more informative one, as you suggested. Sorry, I misread your patch. So you actually completely remove the defvar if var was already defconst'd. That's an attractive approach, tho it's a bit risky, because byte-compile-const-variables only contains variables that "may have been defconst'd", not those that "we know for sure have been defconst'd". E.g. try it with code such as (when toto (defconst foo bar)) (defvar foo nil) with your patch, you'd end up with `foo' undefined in the case where toto is nil. It may seem like a braindead case, but when comparing "broken code in corner cases" vs "incorrect warning in some cases", I prefer the incorrect warning. >> - Finally, if the defconst+defvar is really what you want to do (as in >> the present case where the defconst and the defvar both come from >> macros that we may not want to change), how do you silence >> the warning? > The same way you silence the current one about variable assignment to > a constant. :) The difference is that the current one is incorrect, so the act of silencing it needs to be done in bytecomp.el. Whereas yours is meant to be correct, in which case silencing it would need to be done by tweaking the code and/or using with-no-warnings (which I profoundly disklike). > I think it would be better to change define-derived-mode to not defvar > the map if it is already defined; I don't know how to do that. > or to change easy-mode-defmap to not use defconst That might be an acceptable workaround. But the underlying problem will still be there. > (are you saying users should not add key bindings to > log-edit-mode-map, for example?). That's unrelated: defconst only says that the variable's binding won't change, not that the object it is bound to is immutable. My locally hacked Emacs has "strong defconst" which really makes the variable immutable, so (setq log-edit-mode-map <blabla>) signals an error, but (define-key log-edit-mode-map [foo] 'bar) still works just fine. Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-17 14:02 ` Stefan Monnier @ 2009-09-18 22:25 ` Glenn Morris 2009-09-19 0:04 ` Stephen J. Turnbull 2009-09-19 0:43 ` Stefan Monnier 0 siblings, 2 replies; 13+ messages in thread From: Glenn Morris @ 2009-09-18 22:25 UTC (permalink / raw) To: Stefan Monnier; +Cc: Eli Zaretskii, emacs-devel Stefan Monnier wrote: > It may seem like a braindead case, but when comparing "broken code > in corner cases" vs "incorrect warning in some cases", I prefer the > incorrect warning. Definitely. >> I think it would be better to change define-derived-mode to not defvar >> the map if it is already defined; > > I don't know how to do that. Oh well, that makes two of us. >> or to change easy-mode-defmap to not use defconst > > That might be an acceptable workaround. But the underlying problem will > still be there. But under a carpet. :) >> (are you saying users should not add key bindings to >> log-edit-mode-map, for example?). > > That's unrelated: defconst only says that the variable's binding won't > change, not that the object it is bound to is immutable. I don't understand that distinction. I just read the doc of defconst: The intent is that neither programs nor users should ever change this value. which seems clear to me. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-18 22:25 ` Glenn Morris @ 2009-09-19 0:04 ` Stephen J. Turnbull 2009-09-19 0:43 ` Stefan Monnier 1 sibling, 0 replies; 13+ messages in thread From: Stephen J. Turnbull @ 2009-09-19 0:04 UTC (permalink / raw) To: Glenn Morris; +Cc: Eli Zaretskii, Stefan Monnier, emacs-devel Glenn Morris writes: > Stefan Monnier wrote: > > That's unrelated: defconst only says that the variable's binding won't > > change, not that the object it is bound to is immutable. > > I don't understand that distinction. I just read the doc of defconst: > > The intent is that neither programs nor users should ever change > this value. It's the same distinction as between "const *" and "* const" in C. The value of the variable is a constant object, but that object may be mutable if it is a container. Under this definition, if you were implementing Emacs Lisp in Emacs Lisp, it would make sense to implement the obarray as a defconst. (You probably already knew that, but your question literally says "that distinction", not "why you make that distinction".) I would like to know "why?" though. XEmacs's docstring says "The intent is that programs do not change this value, but users may." The awkward expression of the Emacs docstring suggests that the XEmacs version is indeed the original. Can somebody explain why it was changed? Is Emacs moving toward true constants? (Eg, in r/o memory?) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-18 22:25 ` Glenn Morris 2009-09-19 0:04 ` Stephen J. Turnbull @ 2009-09-19 0:43 ` Stefan Monnier 1 sibling, 0 replies; 13+ messages in thread From: Stefan Monnier @ 2009-09-19 0:43 UTC (permalink / raw) To: Glenn Morris; +Cc: Eli Zaretskii, emacs-devel >>> (are you saying users should not add key bindings to >>> log-edit-mode-map, for example?). >> That's unrelated: defconst only says that the variable's binding won't >> change, not that the object it is bound to is immutable. > I don't understand that distinction. It's the same distinction that makes it possible for a function to modify an array passed as argument in C, even though arguments are passed "by value" (i.e. it receives a copy of the pointer to the array, so it can't change which array is pointed, but it can modify the array's contents). Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Warnings about keymaps 2009-09-14 21:46 ` Stefan Monnier 2009-09-15 2:45 ` Glenn Morris @ 2009-09-15 15:13 ` Richard Stallman 1 sibling, 0 replies; 13+ messages in thread From: Richard Stallman @ 2009-09-15 15:13 UTC (permalink / raw) To: Stefan Monnier; +Cc: eliz, emacs-devel Should there be a way to undo the "constant" marking that defconst makes? ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2009-09-19 0:43 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-11 9:04 Warnings about keymaps Eli Zaretskii 2009-09-11 17:34 ` Glenn Morris 2009-09-11 17:43 ` Stefan Monnier 2009-09-14 19:10 ` Glenn Morris 2009-09-14 21:46 ` Stefan Monnier 2009-09-15 2:45 ` Glenn Morris 2009-09-15 13:40 ` Stefan Monnier 2009-09-17 7:32 ` Glenn Morris 2009-09-17 14:02 ` Stefan Monnier 2009-09-18 22:25 ` Glenn Morris 2009-09-19 0:04 ` Stephen J. Turnbull 2009-09-19 0:43 ` Stefan Monnier 2009-09-15 15:13 ` Richard Stallman
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).