Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Reiner Steib]

On Mon, Oct 22 2007, Katsumi Yamaoka wrote:

>>>>>> Dan Nicolaescu wrote:
>> The subject is about a new warning produced by the byte
>> compiler. There's a lot of those for gnus.
>> Assuming that all emacs versions that gnus wants to support have
>> either mapc or dolist, can somebody please take care of these
>> warnings?
>
>> Thanks
>
> With No Gnus v0.7 I did `cd lisp; make warn' and tried replacing
> of `mapcar' with `mapc', `dolist', or `while' loops for a while,
> and realized it's not easy and not safe.  Ones who do it will
> need to study what things using `mapcar' do in all cases.  For
> instance, `mapcar' used in `gnus-group-highlight-line' cannot be
> replaced with `mapc' even if the compiler warns it.  

Could the byte-compiler be improved not to warn about such cases?
Here's the relevant code:

(defun gnus-group-highlight-line ()
  "Highlight the current line according to `gnus-group-highlight'."
  (let* ([...]
	 (mailp (apply 'append
		       (mapcar
			(lambda (x)
			  (memq x (assoc (symbol-name
					  (car (or method gnus-select-method)))
					 gnus-valid-select-methods)))
			'(mail post-mail))))
	 (level (or (gnus-info-level info) gnus-level-killed))

> In addition, we'd better use `while' loops (or `dolist') rather than
> `mapc' because it is faster, though it needs more work.  I think
> it's worth working anyway but it will take time.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Dan Nicolaescu]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

  > On Mon, Oct 22 2007, Katsumi Yamaoka wrote:
  > 
  > >>>>>> Dan Nicolaescu wrote:
  > >> The subject is about a new warning produced by the byte
  > >> compiler. There's a lot of those for gnus.
  > >> Assuming that all emacs versions that gnus wants to support have
  > >> either mapc or dolist, can somebody please take care of these
  > >> warnings?
  > >
  > >> Thanks
  > >
  > > With No Gnus v0.7 I did `cd lisp; make warn' and tried replacing
  > > of `mapcar' with `mapc', `dolist', or `while' loops for a while,
  > > and realized it's not easy and not safe.  Ones who do it will
  > > need to study what things using `mapcar' do in all cases.  For
  > > instance, `mapcar' used in `gnus-group-highlight-line' cannot be
  > > replaced with `mapc' even if the compiler warns it.  
  > 
  > Could the byte-compiler be improved not to warn about such cases?
  > Here's the relevant code:
  > 
  > (defun gnus-group-highlight-line ()
  >   "Highlight the current line according to `gnus-group-highlight'."
  >   (let* ([...]
  > 	 (mailp (apply 'append
  > 		       (mapcar
  > 			(lambda (x)
  > 			  (memq x (assoc (symbol-name
  > 					  (car (or method gnus-select-method)))
  > 					 gnus-valid-select-methods)))
  > 			'(mail post-mail))))
  > 	 (level (or (gnus-info-level info) gnus-level-killed))

It seems that `mailp' is not used in that function. Adding a use for
it makes the warning go away...

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Reiner Steib]

On Mon, Oct 22 2007, Dan Nicolaescu wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
>   > On Mon, Oct 22 2007, Katsumi Yamaoka wrote:
>   > > For instance, `mapcar' used in `gnus-group-highlight-line'
>   > > cannot be replaced with `mapc' even if the compiler warns it.
>   > 
>   > Could the byte-compiler be improved not to warn about such cases?
>   > Here's the relevant code:
>   > 
>   > (defun gnus-group-highlight-line ()
>   >   "Highlight the current line according to `gnus-group-highlight'."
[...]
> It seems that `mailp' is not used in that function. 

`mailp' is used implicitly through the variable `gnus-group-highlight'
which is let-bound to `list'.  `list' is used in the body of the
let-form.

> Adding a use for it makes the warning go away...

Hm, I can't reproduce the warning using `emacs-lisp-byte-compile' with
GNU Emacs 23.0.50.6 (i686-pc-linux-gnu, GTK+ Version 2.10.6) of 2007-10-20.
Too old?

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Dan Nicolaescu]

Reiner Steib <reinersteib+gmane@imap.cc> writes:

  > On Mon, Oct 22 2007, Dan Nicolaescu wrote:
  > 
  > > Reiner Steib <reinersteib+gmane@imap.cc> writes:
  > >   > On Mon, Oct 22 2007, Katsumi Yamaoka wrote:
  > >   > > For instance, `mapcar' used in `gnus-group-highlight-line'
  > >   > > cannot be replaced with `mapc' even if the compiler warns it.
  > >   > 
  > >   > Could the byte-compiler be improved not to warn about such cases?
  > >   > Here's the relevant code:
  > >   > 
  > >   > (defun gnus-group-highlight-line ()
  > >   >   "Highlight the current line according to `gnus-group-highlight'."
  > [...]
  > > It seems that `mailp' is not used in that function. 
  > 
  > `mailp' is used implicitly through the variable `gnus-group-highlight'
  > which is let-bound to `list'.  `list' is used in the body of the
  > let-form.
  > 
  > > Adding a use for it makes the warning go away...
  > 
  > Hm, I can't reproduce the warning using `emacs-lisp-byte-compile' with
  > GNU Emacs 23.0.50.6 (i686-pc-linux-gnu, GTK+ Version 2.10.6) of 2007-10-20.
  > Too old?

I can't either. I thought that adding a use for mailp fixed the
warning for me, I didn't try it before that.

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Glenn Morris]

Dan Nicolaescu wrote:

> Reiner Steib <reinersteib+gmane@imap.cc> writes:
[...]
>   > Hm, I can't reproduce the warning using `emacs-lisp-byte-compile' with
>   > GNU Emacs 23.0.50.6 (i686-pc-linux-gnu, GTK+ Version 2.10.6) of 2007-10-20.
>   > Too old?
>
> I can't either.

I see the warning only when bootstrapping (in an up-to-date CVS). If I
repeat the same commands as used during bootstrapping, except using
the dumped "emacs" instead of "bootstrap-emacs", I still don't see the
warning:

  cd lisp
  rm gnus/gnus-group.elc
  EMACSLOADPATH=$PWD ../src/emacs \
   -batch --no-site-file --multibyte -f batch-byte-compile-if-not-done \
   gnus/gnus-group.el

Something to do with custom perhaps?

Anyway, regardless of this, from having replaced some mapcars in the
non-gnus parts of the trunk, I can say that the replacement is not
just something that can be done automatically, someone should look at
the actual code in each case.

It shouldn't take very long, and ought to eliminate ~ 100/370 of the
current bootstrap warnings. (There's not much point if the gnus in the
Emacs CVS will soon be replaced with gnus trunk though.)

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Miles Bader]

Reiner Steib <reinersteib+gmane@imap.cc> writes:
>> It seems that `mailp' is not used in that function. 
>
> `mailp' is used implicitly through the variable `gnus-group-highlight'
> which is let-bound to `list'.  `list' is used in the body of the
> let-form.

Ugh.... that's awful.

You should `defvar' mailp then.

-miles
-- 
Everywhere is walking distance if you have the time.  -- Steven Wright

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Richard Stallman]

    You should `defvar' mailp then.

It would be unclean for Gnus to put a defvar on a general short name
like `mailp'.  That is why I think this needs a total rewrite.

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Miles Bader]

Richard Stallman <rms@gnu.org> writes:
>     You should `defvar' mailp then.
>
> It would be unclean for Gnus to put a defvar on a general short name
> like `mailp'.  That is why I think this needs a total rewrite.

In Common-Lisp, I think you use (declare (special VAR ...)) to indicate
a localized use of dynamic binding (as the first thing inside the let
form where it is bound).

Since Elisp already uses `declare' for some other things, it would seem
reasonable to also use it to inform the byte compiler that an apparently
local variable is known to be accessed non-locally.

[That warning is useful -- people often seem to forget to remove unused
let-bindings when the code that uses them is rewritten...]

-Miles

-- 
"Whatever you do will be insignificant, but it is very important that
 you do it."  Mahatma Gandhi

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Richard Stallman]

    In Common-Lisp, I think you use (declare (special VAR ...)) to indicate
    a localized use of dynamic binding (as the first thing inside the let
    form where it is bound).

    Since Elisp already uses `declare' for some other things, it would seem
    reasonable to also use it to inform the byte compiler that an apparently
    local variable is known to be accessed non-locally.

I would not mind adding this feature to Emacs Lisp.
But I still think that part of Gnus should get a total rewrite
using a different mechanism.

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Richard Stallman]

    >   > (defun gnus-group-highlight-line ()
    >   >   "Highlight the current line according to `gnus-group-highlight'."
    [...]
    > It seems that `mailp' is not used in that function. 

    `mailp' is used implicitly through the variable `gnus-group-highlight'
    which is let-bound to `list'.  `list' is used in the body of the
    let-form.

    > Adding a use for it makes the warning go away...

If the byte compiler warns about that case, it is being too ambitious.
Thus, if the value of an expression is stored into a variable, the
compiler should consider the value "used".

So if this warning really happens, it is a bug.
Can it be reproduced?

Risky variables in Gnus (was: Warning: `mapcar' called for effect; use `mapc' or `dolist')

[Reiner Steib]

On Tue, Oct 23 2007, Richard Stallman wrote:

>     >   > (defun gnus-group-highlight-line ()
>     >   >   "Highlight the current line according to `gnus-group-highlight'."
>     [...]
>     > It seems that `mailp' is not used in that function. 
>
>     `mailp' is used implicitly through the variable `gnus-group-highlight'
>     which is let-bound to `list'.  `list' is used in the body of the
>     let-form.
[...]
> I added a `risky-local-variable' property to `gnus-group-highlight'.

Fine with me.  But I can't imagine a way to exploit this.  Setting
`gnus-group-highlight' buffer-locally (using local variables in a
file) won't effect Gnus *Group* buffer where the variable is
evaluated.

> Are there any other such variables in Gnus which contain forms to
> evaluate?

Yes, a quick «grep -inH -e ':tag "Form"' *.el» [1] showed some hits
(there are more probably).  However, I neither can see a way to
maliciously manipulate these variables through file local variables.
I someone can think of a way, please let me know.  I think the vast
majority of Gnus functions are _not_ evaluated in file-visiting
buffers.  But it's fine with me to mark all these variables as risky.

Here's a list of such variable I found so far: `gnus-button-alist',
`gnus-header-button-alist', `gnus-group-charter-alist',
`gnus-group-highlight', `gnus-group-icon-list',
`gnus-summary-highlight', `mm-charset-eval-alist'.

Bye, Reiner.

--8<---------------cut here---------------start------------->8---
-*- mode: grep; default-directory: "~/src/links/plain_No/lisp/" -*-
Grep started at Sat Oct 27 12:29:54

grep -inH -e ':tag "Form"' *.el
gnus-art.el:7181:		       (sexp :tag "Form")
gnus-art.el:7220:		       (sexp :tag "Form")
gnus.el:1482:  :type '(repeat (cons (string :tag "Hierarchy") (sexp :tag "Form"))))
gnus-group.el:384:  :type '(repeat (cons (sexp :tag "Form") face)))
gnus-group.el:422:  :type '(repeat (cons (sexp :tag "Form") file)))
gnus-sum.el:1081:  :type '(repeat (cons (sexp :tag "Form" nil)
mm-util.el:431:			     (symbol :tag "form"))))

Grep finished (matches found) at Sat Oct 27 12:29:55
--8<---------------cut here---------------end--------------->8---
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Risky variables in Gnus (was: Warning: `mapcar' called for effect; use `mapc' or `dolist')

[Richard Stallman]

      I think the vast
    majority of Gnus functions are _not_ evaluated in file-visiting
    buffers.  But it's fine with me to mark all these variables as risky.

It may be unnecessary, but we may as well do it.
Would you please do it?

Re: Risky variables in Gnus

[Reiner Steib]

On Sun, Oct 28 2007, Richard Stallman wrote:

>     I think the vast majority of Gnus functions are _not_ evaluated
>     in file-visiting buffers.  But it's fine with me to mark all
>     these variables as risky.
>
> It may be unnecessary, but we may as well do it.
> Would you please do it?

Yes, I will.  If anyone finds more of these, please mark them as risky
or let me know.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Risky variables in Gnus

[Reiner Steib]

On Sun, Oct 28 2007, Reiner Steib wrote:

> On Sun, Oct 28 2007, Richard Stallman wrote:
>
>>     I think the vast majority of Gnus functions are _not_ evaluated
>>     in file-visiting buffers.  But it's fine with me to mark all
>>     these variables as risky.
>>
>> It may be unnecessary, but we may as well do it.
>> Would you please do it?
>
> Yes, I will.  

Done in Gnus CVS.  (Should be synced to Emacs_22 and trunk.)

> If anyone finds more of these, please mark them as risky or let me
> know.

Any suggestions?

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Richard Stallman]

[I sent this message a week ago but did not get a response.]

    >   > (defun gnus-group-highlight-line ()
    >   >   "Highlight the current line according to `gnus-group-highlight'."
    [...]
    > It seems that `mailp' is not used in that function. 

    `mailp' is used implicitly through the variable `gnus-group-highlight'
    which is let-bound to `list'.  `list' is used in the body of the
    let-form.

    > Adding a use for it makes the warning go away...

If the byte compiler warns about that case, it is being too ambitious.
Thus, if the value of an expression is stored into a variable, the
compiler should consider the value "used".

So if this warning really happens, it is a bug.
Can it be reproduced?

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Reiner Steib]

On Wed, Oct 31 2007, Richard Stallman wrote:

>     >   > (defun gnus-group-highlight-line ()
>     >   >   "Highlight the current line according to `gnus-group-highlight'."
>     [...]
>     > It seems that `mailp' is not used in that function. 
>
>     `mailp' is used implicitly through the variable `gnus-group-highlight'
>     which is let-bound to `list'.  `list' is used in the body of the
>     let-form.
>
>     > Adding a use for it makes the warning go away...
>
> If the byte compiler warns about that case, it is being too ambitious.
> Thus, if the value of an expression is stored into a variable, the
> compiler should consider the value "used".
>
> So if this warning really happens, it is a bug.
> Can it be reproduced?

I can't reproduce it.  I just updated and bootstrapped.  There was no
warning for `gnus-group.el':

  Compiling [...]/cvs-HEAD/emacs/lisp/./gnus/gnus-group.el
  Wrote [...]/cvs-HEAD/emacs/lisp/gnus/gnus-group.elc

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Glenn Morris]

Richard Stallman wrote:

> If the byte compiler warns about that case, it is being too
> ambitious. Thus, if the value of an expression is stored into a
> variable, the compiler should consider the value "used".
>
> So if this warning really happens, it is a bug. Can it be
> reproduced?

There's nothing to worry about AFAICS:

(let ((foo (mapcar (lambda (e) e) '(a b c)))))     ; warning

(let ((foo (mapcar (lambda (e) e) '(a b c))))      ; no warning
  foo)                            

(defvar foo nil)
(defun foo-func ()
  foo)
(let ((foo (mapcar (lambda (e) e) '(a b c))))      ; no warning
  (foo-func))

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Katsumi Yamaoka]

>>>>> In http://article.gmane.org/gmane.emacs.gnus.general/65434
>>>>>	I wrote:

> For instance, `mapcar' used in `gnus-group-highlight-line'
> cannot be replaced with `mapc' even if the compiler warns it.

>>>>> In http://article.gmane.org/gmane.emacs.gnus.general/65439
>>>>>	Reiner Steib wrote:

> `mailp' is used implicitly through the variable `gnus-group-highlight'
> which is let-bound to `list'.  `list' is used in the body of the
> let-form.

>>>>> Richard Stallman wrote:

> So if this warning really happens, it is a bug.
> Can it be reproduced?

When I wrote the first message I saw that the byte compiler
warned about the usage of mapcar in `gnus-group-highlight-line',
but it has gone after I rebuilt Emacs after I wrote that message.
I also tried Emacs built with the source of two weeks ago, but it
didn't happen.  That's a mystery, but I don't intend to inquire
into it further.

Regards,

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Reiner Steib]

On Mon, Oct 22 2007, Katsumi Yamaoka wrote:

>>>>>> Dan Nicolaescu wrote:
>> Assuming that all emacs versions that gnus wants to support have
>> either mapc or dolist, can somebody please take care of these
>> warnings?
[...]
> With No Gnus v0.7 I did `cd lisp; make warn' and tried replacing
> of `mapcar' with `mapc', `dolist', or `while' loops for a while,
> and realized it's not easy and not safe.  Ones who do it will
> need to study what things using `mapcar' do in all cases.

Thank you for working on this.  

However, you also committed it to v5-10.  As using `mapcar' is not a
bug and the changes might break the code, I'd prefer to install these
changes only in Gnus trunk (which will eventually be synced to Emacs
trunk), but not in v5-10 (and Emacs 22) unless there are other
arguments.  An argument could be to keep the code in trunk and v5-10
as close as possible.

A counter argument is that Lars suggested not to break compatibility
(with Emacs 20.7) actively:

,----[ http://thread.gmane.org/gmane.emacs.gnus.general/62286/focus=62770 ]
| Reiner Steib <reinersteib+gmane <at> imap.cc> writes:
| 
| > Do you suggest to rip out Emacs 20/XEmacs 21.1 stuff systematically
| > like it was done after the start of No Gnus (mostly by Jesper and
| > Katsumi [2]) or only drop it on occasion?
| 
| For 5.10, I think it's better to just let it decay instead of removing
| it actively.
`----

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Warning: `mapcar' called for effect; use `mapc' or `dolist'

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:

> However, you also committed it to v5-10.  As using `mapcar' is not a
> bug and the changes might break the code, I'd prefer to install these
> changes only in Gnus trunk (which will eventually be synced to Emacs
> trunk), but not in v5-10 (and Emacs 22) unless there are other
> arguments.  An argument could be to keep the code in trunk and v5-10
> as close as possible.

Oops, I misunderstood what the v5-10 branch is for.  I'll revert
the last change in the branch soon.  Thanks.