weird desktop.el change

weird desktop.el change

[Miles Bader]

Hi,

You made this change to "lisp/desktop.el":

   revision 1.107
   date: 2007-04-06 18:35:23 +0000;  author: cyd;  state: Exp;  lines: +4 -4
   (desktop-create-buffer, desktop-save): Revert 2004-11-12 change.

The "2004-11-12 change" seems to be this:

   revision 1.76
   date: 2004-11-12 16:54:30 +0000;  author: eliz;  state: Exp;  lines: +13 -13
   (desktop-create-buffer, desktop-save): Avoid some consing by using mapc
   instead of mapcar.

A quick glance suggests that Eli's change was correct (the return values
of the calls to mapc/mapcar are all ignored, so using mapc seems OK),
and does indeed avoid some consing, so why did you revert it?

[Sorry if this was discussed -- a search on the emacs-devel archives
didn't turn up anything obvious.]

Thanks,

-Miles

-- 
"Suppose He doesn't give a shit?  Suppose there is a God but He
just doesn't give a shit?"  [George Carlin]

Re: weird desktop.el change

[Juanma Barranquero]

On 4/11/07, Miles Bader <miles@gnu.org> wrote:

> A quick glance suggests that Eli's change was correct (the return values
> of the calls to mapc/mapcar are all ignored, so using mapc seems OK),
> and does indeed avoid some consing, so why did you revert it?

That's one of Kevin Rodgers changes that has been reverted to allow
the release to go on.

This is the relevant original ChangeLog entry:

2004-11-12  Kevin Rodgers  <ihs_4664@yahoo.com>  (tiny change)

        * desktop.el (desktop-create-buffer, desktop-save): Avoid some
        consing by using mapc instead of mapcar.


             Juanma

Re: weird desktop.el change

[Miles Bader]

"Juanma Barranquero" <lekktu@gmail.com> writes:
>> A quick glance suggests that Eli's change was correct (the return values
>> of the calls to mapc/mapcar are all ignored, so using mapc seems OK),
>> and does indeed avoid some consing, so why did you revert it?
>
> That's one of Kevin Rodgers changes that has been reverted to allow
> the release to go on.

Er, OK...  does such a trivial and purely mechanical change really
count?  What if I make an equivalent change now?

-Miles

-- 
Come now, if we were really planning to harm you, would we be waiting here,
 beside the path, in the very darkest part of the forest?

Re: weird desktop.el change

[Chong Yidong]

Miles Bader <miles.bader@necel.com> writes:

> "Juanma Barranquero" <lekktu@gmail.com> writes:
>>> A quick glance suggests that Eli's change was correct (the return values
>>> of the calls to mapc/mapcar are all ignored, so using mapc seems OK),
>>> and does indeed avoid some consing, so why did you revert it?
>>
>> That's one of Kevin Rodgers changes that has been reverted to allow
>> the release to go on.
>
> Er, OK...  does such a trivial and purely mechanical change really
> count?  What if I make an equivalent change now?

I have no idea.

Re: weird desktop.el change

[Stephen J. Turnbull]

Miles Bader writes:

 > "Juanma Barranquero" <lekktu@gmail.com> writes:
 > >> A quick glance suggests that Eli's change was correct (the return values
 > >> of the calls to mapc/mapcar are all ignored, so using mapc seems OK),
 > >> and does indeed avoid some consing, so why did you revert it?
 > >
 > > That's one of Kevin Rodgers changes that has been reverted to allow
 > > the release to go on.
 > 
 > Er, OK...  does such a trivial and purely mechanical change really
 > count?  What if I make an equivalent change now?

The question is, is there another way to express "mapc"?  If not,
Kevin had no choice of expression, and there was no original
expression, and thus no copyright.  KR cannot have a copyright on the
idea of substituting an algorithm which never conses rather than
throwing away unwanted conses ex post, only on the expression of it.

AIUI IMHO IANAL.

Re: weird desktop.el change

[Richard Matthew Stallman]

    Er, OK...  does such a trivial and purely mechanical change really
    count?  What if I make an equivalent change now?

It isn't purely mechanical, and if you made the same change because you
saw Kevin's, it would be copying.

Re: weird desktop.el change

[Miles Bader]

Richard Matthew Stallman <rms@gnu.org> writes:
>     Er, OK...  does such a trivial and purely mechanical change really
>     count?  What if I make an equivalent change now?
>
> It isn't purely mechanical, and if you made the same change because you
> saw Kevin's, it would be copying.

The process of substituting "mapc" for "mapcar" when the result isn't
used is not only a hugely widespread tweak in the lisp community (a "no
brainer" as you might call it), indeed it's an obvious optimization for
even a very stupid lisp compiler...

I think in no way could it be said to be require any creative thinking.

-Miles

-- 
/\ /\
(^.^)
(")")
*This is the cute kitty virus, please copy this into your sig so it can spread.

Re: weird desktop.el change

[Markus Triska]

Miles Bader <miles@gnu.org> writes:

> The process of substituting "mapc" for "mapcar" when the result
> isn't used is not only a hugely widespread tweak in the lisp
> community (a "no brainer" as you might call it), indeed it's an
> obvious optimization for even a very stupid lisp compiler...

Available with this patch:

2007-04-13  Markus Triska  <markus.triska@gmx.at>

	* emacs-lisp/byte-opt.el (byte-optimize-form-code-walker): rewrite
	`mapcar' to `mapc' when called for effect

Index: byte-opt.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/emacs-lisp/byte-opt.el,v
retrieving revision 1.94
diff -c -r1.94 byte-opt.el
*** byte-opt.el	11 Apr 2007 17:10:42 -0000	1.94
--- byte-opt.el	13 Apr 2007 16:48:08 -0000
***************
*** 408,413 ****
--- 408,418 ----
  						 (prin1-to-string clause))
  			      clause))
  			 (cdr form))))
+ 	  ((and for-effect (eq fn 'mapcar))
+ 	   (when (/= (length form) 3)
+ 	     (byte-compile-warn "wrong number of arguments for `mapcar'"))
+ 	   (byte-compile-warn "`mapcar' called for effect; using `mapc'")
+ 	   (byte-optimize-form (cons 'mapc (cdr form)) for-effect))
  	  ((eq fn 'progn)
  	   ;; as an extra added bonus, this simplifies (progn <x>) --> <x>
  	   (if (cdr (cdr form))

Re: weird desktop.el change

[Stefan Monnier]

>> The process of substituting "mapc" for "mapcar" when the result
>> isn't used is not only a hugely widespread tweak in the lisp
>> community (a "no brainer" as you might call it), indeed it's an
>> obvious optimization for even a very stupid lisp compiler...

> Available with this patch:

> 2007-04-13  Markus Triska  <markus.triska@gmx.at>

> 	* emacs-lisp/byte-opt.el (byte-optimize-form-code-walker): rewrite
> 	`mapcar' to `mapc' when called for effect

Please, let's not add this before the release.


        Stefan


PS: I'm not even sure it's a good optimization: in 99% of the cases it would
be better to educate the programmer about mapc by emitting a warning, and in
99% of those cases, it'd be even better to replace the mapc(ar) by
a dolist loop.

Re: weird desktop.el change

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> PS: I'm not even sure it's a good optimization: in 99% of the cases
> it would be better to educate the programmer about mapc by emitting
> a warning,

The patch does that.

> and in 99% of those cases, it'd be even better to replace the
> mapc(ar) by a dolist loop.

That's quite unrelated.

Re: weird desktop.el change

[Richard Stallman]

    > 2007-04-13  Markus Triska  <markus.triska@gmx.at>

    > 	* emacs-lisp/byte-opt.el (byte-optimize-form-code-walker): rewrite
    > 	`mapcar' to `mapc' when called for effect

    Please, let's not add this before the release.

I agree.

    PS: I'm not even sure it's a good optimization: in 99% of the cases it would
    be better to educate the programmer about mapc by emitting a warning, and in
    99% of those cases, it'd be even better to replace the mapc(ar) by
    a dolist loop.

Markus says his code does issue a warning.  However, I think it would
be better to implement this warning in the code generation stage so as
to give the warning the correct line number.

I think that would make the optimization unnecessary.

Re: weird desktop.el change

[Markus Triska]

Richard Stallman <rms@gnu.org> writes:

> I think it would be better to implement this warning in the code
> generation stage so as to give the warning the correct line number.

I will revisit this issue after the release. For now, I compiled a
list of all opportunities for this optimisation in Emacs trunk, found
with the following change to bytecomp.el. It only emits a warning and
does not change the byte code. Most line numbers are accurate; in some
cases they may point to the wrong `mapcar' call though.

*** bytecomp.el	11 Apr 2007 12:56:05 +0200	2.198
--- bytecomp.el	15 Apr 2007 00:18:34 +0200	
***************
*** 2831,2836 ****
--- 2831,2839 ----
  (defun byte-compile-normal-call (form)
    (if byte-compile-generate-call-tree
        (byte-compile-annotate-call-tree form))
+   (when (and for-effect (eq (car form) 'mapcar))
+     (byte-compile-set-symbol-position 'mapcar)
+     (byte-compile-warn "`mapcar' called for effect; use `mapc' instead"))
    (byte-compile-push-constant (car form))
    (mapc 'byte-compile-form (cdr form))	; wasteful, but faster.
    (byte-compile-out 'byte-call (length (cdr form))))


File                    Line(s)
==================      ==================

byte-opt.el             1999
bytecomp.el             4239
cc-mode.el              850,1439
allout.el               1133,5465
ansi-color.el           567,571,580
autoinsert.el           275
bookmark.el             1572,1789
desktop.el              871,1031
dired-aux.el            1335
dired.el                3326
ediff-diff.el           351,361,364
ediff-mult.el           639,924,1308,1369,1369
ediff-ptch.el           338,335
ediff-util.el           1320,1344,2442,2445,3746
emerge.el               1889
ffap.el                 485
filecache.el            287,389,405,446,459,722,771
files.el                646
find-lisp.el            273
finder.el               137
follow.el               394,400
frame.el                453,832
help.el                 335
hi-lock.el              490
ido.el                  3357,3606,3654
jka-cmpr-hook.el        100
printing.el             5293,5323
ps-print.el             5020,5662,6435,6437,6436,6437
simple.el               5370
startup.el              977
tempo.el                318,463,559,578
tumme.el                825,902,2148
vc-hooks.el             354
vc.el                   1599,1622,1666,2289,2622,2650
woman.el                1537
calc-ext.el             621,655,1280
calc-help.el            422,441
calc-misc.el            148
calc-store.el           175,178
calc-stuff.el           194
calc-units.el           673
calc.el                 921,978,978,987,1012,1060,1082,1183,1187,1366
hilit19.el              668,978,1026
emacsbug.el             152
feedmail.el             1591,1838
reporter.el             255
rmail.el                1457
supercite.el            1046,1057,1067
authors.el              669
cl.el                   596,641
cust-print.el           247,262,276
disass.el               253
easy-mmode.el           458
edebug.el               4424
elint.el                221,545,569,668,770
elp.el                  618
generic.el              208
re-builder.el           518
regi.el                 169
sregex.el               568
solitaire.el            403,449
zone.el                 401
icalendar.el            1534,1640
gnus-agent.el           686,1004,1278,2038,2091,2096,3487
gnus-art.el             3753,7273
gnus-diary.el           311
gnus-group.el           1629,2204,2405,4107
gnus-int.el             169,199,229,285,292,299,306,316,334,344,404,513,
                               534,584,642,688
gnus-msg.el             1577,1578,1584
gnus-nocem.el           194
gnus-registry.el        352
gnus-spec.el            300
gnus-srvr.el            395,505,525,537,553,577,580,629,785,1006
gnus-start.el           968,1466,1475,1781,3026
gnus-sum.el             1482,1566,4132,5847,9275,10470,11286
gnus-topic.el           247,354,1300,1321
gnus.el                 2740,3522
imap.el                 1032,1031,2743
legacy-gnus-agent.el    114,130
message.el              7056
mm-util.el              33,983,1066
mml.el                  157,652,813
nndiary.el              1699
nnfolder.el             1200
nnimap.el               823,1145,1210,1561,1565,1654
nnmail.el               1675
nnmaildir.el            242,710,740,760,759,824,821,860,928,936,1090,1147,
                               1540,1544,1548,1584,1593,1611
nnml.el                 945
nnvirtual.el            342,679,738
nnweb.el                557
sieve-manage.el         375
webmail.el              199
artist.el               1565,1614,1703,1740,3163,3223,3225,3362,5364
flyspell.el             613,632
org.el                  11856,17011,17071,17477
reftex-cite.el          712,957
reftex-ref.el           670
reftex-sel.el           424,646
reftex-toc.el           619
reftex.el               862,1344
table.el                1379,1412,1446,1472,1651,3144,3226,3923,3925,4095
ethio-util.el           1064
mule-diag.el            842
erc-backend.el          658
erc-track.el            672
url-dav.el              936
url-vars.el             65
cua-rect.el             734,840,1404
viper-keym.el           173,213
viper-macs.el           661,665,673,680
viper-util.el           1265
viper.el                795
sun-mouse.el            504
tvi970.el               40
w32-win.el              114
ange-ftp.el             4558,6033
eudc-hotlist.el         72,84
eudc.el                 510,515,543,568,574,644,991,998
eudcb-bbdb.el           78,206
eudcb-ldap.el           133
tramp-vc.el             103
tramp.el                3976,4635,4747,5253
ada-mode.el             1434,1439
ada-prj.el              257,258
cc-styles.el            384,639
cperl-mode.el           1103,5533,7140,7264,7273,7273,7339,7345,7363,7353,7360
delphi.el               1689,1692,1724,1891,1912,1984
ebnf-yac.el             281,278
ebnf2ps.el              4654,5141
ebrowse.el              1151,1641,2259,4151,4166
f90.el                  685
fortran.el              615
idlw-shell.el           3481,3481,4533
idlw-toolbar.el         914,945
idlwave.el              2828,2830,4243,5561,5605,6320,6336,6808,7552,8278,8438
sh-script.el            1892,1899
sql.el                  866
vhdl-mode.el            2814,5299,7148
xscheme.el              104

Re: weird desktop.el change

[Richard Stallman]

    > I think it would be better to implement this warning in the code
    > generation stage so as to give the warning the correct line number.

    I will revisit this issue after the release.

Thank you in advance; please do.