* false warnings when compiling with lexical-binding and cl-lib.
@ 2013-11-21 15:29 Thierry Volpiatto
2013-11-22 1:19 ` Stefan Monnier
0 siblings, 1 reply; 9+ messages in thread
From: Thierry Volpiatto @ 2013-11-21 15:29 UTC (permalink / raw
To: emacs-devel
Hi,
here a sample file containing a function using loop:
--8<---------------cut here---------------start------------->8---
;;; test.el --- test loop -*- lexical-binding: t -*-
(require 'cl-lib)
(defun test-with (l)
(cl-loop with lst
for i in l
unless (member i lst)
collect i into lst
finally return lst))
--8<---------------cut here---------------end--------------->8---
When compiling this file I have a warning about unused arg `lst'.
Is this wanted ?
Thanks.
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-21 15:29 false warnings when compiling with lexical-binding and cl-lib Thierry Volpiatto
@ 2013-11-22 1:19 ` Stefan Monnier
2013-11-22 5:33 ` Thierry Volpiatto
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Stefan Monnier @ 2013-11-22 1:19 UTC (permalink / raw
To: Thierry Volpiatto; +Cc: emacs-devel
> here a sample file containing a function using loop:
> --8<---------------cut here---------------start------------->8---
> ;;; test.el --- test loop -*- lexical-binding: t -*-
> (require 'cl-lib)
> (defun test-with (l)
> (cl-loop with lst
> for i in l
> unless (member i lst)
> collect i into lst
> finally return lst))
> --8<---------------cut here---------------end--------------->8---
> When compiling this file I have a warning about unused arg `lst'.
> Is this wanted ?
No, it's not wanted. But noone so far as been able to come up with
a patch to fix it (the code to fix is the cl-loop macro: look at the
macro-expanded code to see the unused `lst').
Stefan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-22 1:19 ` Stefan Monnier
@ 2013-11-22 5:33 ` Thierry Volpiatto
2013-11-22 7:48 ` Thierry Volpiatto
2013-11-23 6:13 ` Thierry Volpiatto
2 siblings, 0 replies; 9+ messages in thread
From: Thierry Volpiatto @ 2013-11-22 5:33 UTC (permalink / raw
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> here a sample file containing a function using loop:
>
>> --8<---------------cut here---------------start------------->8---
>> ;;; test.el --- test loop -*- lexical-binding: t -*-
>
>> (require 'cl-lib)
>
>> (defun test-with (l)
>> (cl-loop with lst
>> for i in l
>> unless (member i lst)
>> collect i into lst
>> finally return lst))
>> --8<---------------cut here---------------end--------------->8---
>
>> When compiling this file I have a warning about unused arg `lst'.
>> Is this wanted ?
>
> No, it's not wanted. But noone so far as been able to come up with
> a patch to fix it (the code to fix is the cl-loop macro: look at the
> macro-expanded code to see the unused `lst').
Ok thanks.
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-22 1:19 ` Stefan Monnier
2013-11-22 5:33 ` Thierry Volpiatto
@ 2013-11-22 7:48 ` Thierry Volpiatto
2013-11-23 6:13 ` Thierry Volpiatto
2 siblings, 0 replies; 9+ messages in thread
From: Thierry Volpiatto @ 2013-11-22 7:48 UTC (permalink / raw
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> here a sample file containing a function using loop:
>
>> --8<---------------cut here---------------start------------->8---
>> ;;; test.el --- test loop -*- lexical-binding: t -*-
>
>> (require 'cl-lib)
>
>> (defun test-with (l)
>> (cl-loop with lst
>> for i in l
>> unless (member i lst)
>> collect i into lst
>> finally return lst))
>> --8<---------------cut here---------------end--------------->8---
>
>> When compiling this file I have a warning about unused arg `lst'.
>> Is this wanted ?
>
> No, it's not wanted. But noone so far as been able to come up with
> a patch to fix it (the code to fix is the cl-loop macro: look at the
> macro-expanded code to see the unused `lst').
Ok I see:
,----
| (let* ((lst nil) (--cl-var-- l) (i nil) (lst nil))
`----
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-22 1:19 ` Stefan Monnier
2013-11-22 5:33 ` Thierry Volpiatto
2013-11-22 7:48 ` Thierry Volpiatto
@ 2013-11-23 6:13 ` Thierry Volpiatto
2013-11-23 13:51 ` Stefan Monnier
2 siblings, 1 reply; 9+ messages in thread
From: Thierry Volpiatto @ 2013-11-23 6:13 UTC (permalink / raw
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> here a sample file containing a function using loop:
>
>> --8<---------------cut here---------------start------------->8---
>> ;;; test.el --- test loop -*- lexical-binding: t -*-
>
>> (require 'cl-lib)
>
>> (defun test-with (l)
>> (cl-loop with lst
>> for i in l
>> unless (member i lst)
>> collect i into lst
>> finally return lst))
>> --8<---------------cut here---------------end--------------->8---
>
>> When compiling this file I have a warning about unused arg `lst'.
>> Is this wanted ?
>
> No, it's not wanted. But noone so far as been able to come up with
> a patch to fix it (the code to fix is the cl-loop macro: look at the
> macro-expanded code to see the unused `lst').
This patch avoid adding a new binding by the 'into' clause when the
binding have been already added by a 'with' clause.
I wonder if this is correct, real common-lisp return an error in such
case, WDYT?
--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 2209297..63cf7a1 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1564,9 +1564,11 @@ If BODY is `setq', then use SPECS for assignments rather than for bindings."
(defun cl--loop-handle-accum (def &optional func) ; uses loop-*
(if (eq (car cl--loop-args) 'into)
- (let ((var (cl--pop2 cl--loop-args)))
+ (let* ((var (cl--pop2 cl--loop-args))
+ (binding (list (list var def))))
(or (memq var cl--loop-accum-vars)
- (progn (push (list (list var def)) cl--loop-bindings)
+ (member binding cl--loop-bindings)
+ (progn (push binding cl--loop-bindings)
(push var cl--loop-accum-vars)))
var)
(or cl--loop-accum-var
--8<---------------cut here---------------end--------------->8---
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-23 6:13 ` Thierry Volpiatto
@ 2013-11-23 13:51 ` Stefan Monnier
2013-11-23 15:53 ` Thierry Volpiatto
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2013-11-23 13:51 UTC (permalink / raw
To: Thierry Volpiatto; +Cc: emacs-devel
> This patch avoid adding a new binding by the 'into' clause when the
> binding have been already added by a 'with' clause.
> I wonder if this is correct, real common-lisp return an error in such
> case, WDYT?
Oh, wait, then the warning is OK. It indicates that the `with lst' is
extraneous and unused. IOW, there's no bug to fix. We could change
cl-loop to signal an error, but it doesn't seem to be worth the trouble.
Stefan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-23 13:51 ` Stefan Monnier
@ 2013-11-23 15:53 ` Thierry Volpiatto
2013-11-23 16:24 ` Thierry Volpiatto
0 siblings, 1 reply; 9+ messages in thread
From: Thierry Volpiatto @ 2013-11-23 15:53 UTC (permalink / raw
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> This patch avoid adding a new binding by the 'into' clause when the
>> binding have been already added by a 'with' clause.
>> I wonder if this is correct, real common-lisp return an error in such
>> case, WDYT?
>
> Oh, wait, then the warning is OK. It indicates that the `with lst' is
> extraneous and unused. IOW, there's no bug to fix. We could change
> cl-loop to signal an error, but it doesn't seem to be worth the trouble.
Yes, maybe the error message can be a little more explicit, common-lisp send a
message like this:
,----
| Variable LST in INTO clause is a duplicate
| current LOOP context: COLLECT I INTO LST FINALLY.
| [Condition of type SB-INT:COMPILED-PROGRAM-ERROR]
`----
Also the warning is only at compile time, should we return an error on
evaluation ?
This can be detected and handled from the same function
i.e (cl--loop-handle-accum)
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-23 15:53 ` Thierry Volpiatto
@ 2013-11-23 16:24 ` Thierry Volpiatto
2013-11-24 18:08 ` Stefan Monnier
0 siblings, 1 reply; 9+ messages in thread
From: Thierry Volpiatto @ 2013-11-23 16:24 UTC (permalink / raw
To: emacs-devel
Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> This patch avoid adding a new binding by the 'into' clause when the
>>> binding have been already added by a 'with' clause.
>>> I wonder if this is correct, real common-lisp return an error in such
>>> case, WDYT?
>>
>> Oh, wait, then the warning is OK. It indicates that the `with lst' is
>> extraneous and unused. IOW, there's no bug to fix. We could change
>> cl-loop to signal an error, but it doesn't seem to be worth the trouble.
>
> Yes, maybe the error message can be a little more explicit, common-lisp send a
> message like this:
>
> ,----
> | Variable LST in INTO clause is a duplicate
> | current LOOP context: COLLECT I INTO LST FINALLY.
> | [Condition of type SB-INT:COMPILED-PROGRAM-ERROR]
> `----
>
> Also the warning is only at compile time, should we return an error on
> evaluation ?
> This can be detected and handled from the same function
> i.e (cl--loop-handle-accum)
--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 2209297..3faf23b 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1564,9 +1564,13 @@ If BODY is `setq', then use SPECS for assignments rather than for bindings."
(defun cl--loop-handle-accum (def &optional func) ; uses loop-*
(if (eq (car cl--loop-args) 'into)
- (let ((var (cl--pop2 cl--loop-args)))
+ (let* ((var (cl--pop2 cl--loop-args))
+ (binding (list (list var def))))
(or (memq var cl--loop-accum-vars)
- (progn (push (list (list var def)) cl--loop-bindings)
+ (cl-assert (not (member binding cl--loop-bindings))
+ nil "Variable %s in INTO clause is a duplicate"
+ (upcase (symbol-name (caar binding))))
+ (progn (push binding cl--loop-bindings)
(push var cl--loop-accum-vars)))
var)
(or cl--loop-accum-var
--8<---------------cut here---------------end--------------->8---
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: false warnings when compiling with lexical-binding and cl-lib.
2013-11-23 16:24 ` Thierry Volpiatto
@ 2013-11-24 18:08 ` Stefan Monnier
0 siblings, 0 replies; 9+ messages in thread
From: Stefan Monnier @ 2013-11-24 18:08 UTC (permalink / raw
To: Thierry Volpiatto; +Cc: emacs-devel
>> Also the warning is only at compile time, should we return an error on
>> evaluation ?
I don't see why we should bother with an error.
(let* ((a 1) (a 2)) ...)
is not very useful, but it's not an error. The warning we currently
have is not perfect because it applies to the post-macroexpansion code,
so the user may not immediately understand what's going on, but your
suggested patch has the following downsides:
- it breaks previously working code.
- it does not tell you where (approximate line number) the error is located.
So if we want to improve on the current code, I think rather than
`cl-assert' we should use macroexp--warn-and-return (grep for it to see
sample uses). But the patch won't be as trivial.
Stefan
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-24 18:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-21 15:29 false warnings when compiling with lexical-binding and cl-lib Thierry Volpiatto
2013-11-22 1:19 ` Stefan Monnier
2013-11-22 5:33 ` Thierry Volpiatto
2013-11-22 7:48 ` Thierry Volpiatto
2013-11-23 6:13 ` Thierry Volpiatto
2013-11-23 13:51 ` Stefan Monnier
2013-11-23 15:53 ` Thierry Volpiatto
2013-11-23 16:24 ` Thierry Volpiatto
2013-11-24 18:08 ` Stefan Monnier
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.