GNU bug report logs -
#18175
files.el: use mapc in (mapcar 'switch-to-buffer ...)
Previous Next
Reported by: Ivan Shmakov <ivan <at> siamics.net>
Date: Sat, 2 Aug 2014 21:56:02 UTC
Severity: wishlist
Tags: patch
Fixed in version 25.1
Done: Ivan Shmakov <ivan <at> siamics.net>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 18175 in the body.
You can then email your comments to 18175 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Sat, 02 Aug 2014 21:56:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Ivan Shmakov <ivan <at> siamics.net>
:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org
.
(Sat, 02 Aug 2014 21:56:03 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Package: emacs
Severity: wishlist
Given that switch-to-buffer returns its argument, /and/ given
that mapc returns the sequence it’s given, I suggest that the
(mapcar 'switch-to-buffer LIST) forms in lisp/files.el be
replaced with (mapc 'switch-to-buffer LIST), – if only to avoid
the unnecessary consing when the list is effectively copied in
the mapcar case.
The lists mapcar is applied to in such cases are returned from
find-file-noselect, and so, as it seems, are “fresh” ones
anyway.
A possible patch is MIMEd.
--
FSF associate member #7257 http://boycottsystemd.org/ … 3013 B6A0 230E 334A
[Message part 2 (text/x-diff, inline)]
diff --git a/lisp/files.el b/lisp/files.el
index 9272e98..e604ce7 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1428,7 +1428,7 @@ automatically choosing a major mode, use \\[find-file-literally]."
(confirm-nonexistent-file-or-buffer)))
(let ((value (find-file-noselect filename nil nil wildcards)))
(if (listp value)
- (mapcar 'switch-to-buffer (nreverse value))
+ (mapc 'switch-to-buffer (nreverse value))
(switch-to-buffer value))))
(defun find-file-other-window (filename &optional wildcards)
@@ -1451,7 +1451,7 @@ expand wildcards (if any) and visit multiple files."
(progn
(setq value (nreverse value))
(cons (switch-to-buffer-other-window (car value))
- (mapcar 'switch-to-buffer (cdr value))))
+ (mapc 'switch-to-buffer (cdr value))))
(switch-to-buffer-other-window value))))
(defun find-file-other-frame (filename &optional wildcards)
@@ -1474,7 +1474,7 @@ expand wildcards (if any) and visit multiple files."
(progn
(setq value (nreverse value))
(cons (switch-to-buffer-other-frame (car value))
- (mapcar 'switch-to-buffer (cdr value))))
+ (mapc 'switch-to-buffer (cdr value))))
(switch-to-buffer-other-frame value))))
(defun find-file-existing (filename)
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Sun, 03 Aug 2014 00:57:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 18175 <at> debbugs.gnu.org (full text, mbox):
> Given that switch-to-buffer returns its argument, /and/ given
> that mapc returns the sequence it’s given, I suggest that the
> (mapcar 'switch-to-buffer LIST) forms in lisp/files.el be
> replaced with (mapc 'switch-to-buffer LIST), – if only to avoid
> the unnecessary consing when the list is effectively copied in
> the mapcar case.
>
> The lists mapcar is applied to in such cases are returned from
> find-file-noselect, and so, as it seems, are “fresh” ones
> anyway.
Not a good idea, IMHO.
It's not just about performance; it's about coding style.
By using `mapcar' you are signaling that you are interested in the return values of the argument function (and of course the resulting list of them).
By using `mapc' you are signaling that the values returned by the argument function are unimportant (only its side effects are significant).
If you want to improve the performance, and that is the only change you want to make, then please consider another approach.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Sun, 03 Aug 2014 08:56:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 18175 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>>>>> Drew Adams <drew.adams <at> oracle.com> writes:
>> Given that switch-to-buffer returns its argument, /and/ given that
>> mapc returns the sequence it’s given, I suggest that the
>> (mapcar 'switch-to-buffer LIST) forms in lisp/files.el be replaced
>> with (mapc 'switch-to-buffer LIST), – if only to avoid the
>> unnecessary consing when the list is effectively copied in the
>> mapcar case.
>> The lists mapcar is applied to in such cases are returned from
>> find-file-noselect, and so, as it seems, are “fresh” ones anyway.
> Not a good idea, IMHO.
> It's not just about performance; it's about coding style.
> By using `mapcar' you are signaling that you are interested in the
> return values of the argument function (and of course the resulting
> list of them).
> By using `mapc' you are signaling that the values returned by the
> argument function are unimportant (only its side effects are
> significant).
How do you signal that the values returned by the argument
function are unimportant, /and/ that you’re interested in the
/original/ list instead?
> If you want to improve the performance, and that is the only change
> you want to make, then please consider another approach.
Please consider the patch MIMEd. FWIW, it avoids one more cons
in both find-file-other-window and find-file-other-frame.
--
FSF associate member #7257 http://boycottsystemd.org/ … 3013 B6A0 230E 334A
[Message part 2 (text/x-diff, inline)]
diff --git a/lisp/files.el b/lisp/files.el
index 9272e98..c3a6f0d 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1428,7 +1428,7 @@ automatically choosing a major mode, use \\[find-file-literally]."
(confirm-nonexistent-file-or-buffer)))
(let ((value (find-file-noselect filename nil nil wildcards)))
(if (listp value)
- (mapcar 'switch-to-buffer (nreverse value))
+ (mapc 'switch-to-buffer (nreverse value))
(switch-to-buffer value))))
(defun find-file-other-window (filename &optional wildcards)
@@ -1448,10 +1448,10 @@ expand wildcards (if any) and visit multiple files."
(confirm-nonexistent-file-or-buffer)))
(let ((value (find-file-noselect filename nil nil wildcards)))
(if (listp value)
- (progn
- (setq value (nreverse value))
- (cons (switch-to-buffer-other-window (car value))
- (mapcar 'switch-to-buffer (cdr value))))
+ (prog1
+ (setq value (nreverse value))
+ (switch-to-buffer-other-window (car value))
+ (mapc 'switch-to-buffer (cdr value)))
(switch-to-buffer-other-window value))))
(defun find-file-other-frame (filename &optional wildcards)
@@ -1471,10 +1471,10 @@ expand wildcards (if any) and visit multiple files."
(confirm-nonexistent-file-or-buffer)))
(let ((value (find-file-noselect filename nil nil wildcards)))
(if (listp value)
- (progn
- (setq value (nreverse value))
- (cons (switch-to-buffer-other-frame (car value))
- (mapcar 'switch-to-buffer (cdr value))))
+ (prog1
+ (setq value (nreverse value))
+ (switch-to-buffer-other-frame (car value))
+ (mapc 'switch-to-buffer (cdr value)))
(switch-to-buffer-other-frame value))))
(defun find-file-existing (filename)
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Sun, 03 Aug 2014 16:33:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 18175 <at> debbugs.gnu.org (full text, mbox):
> > By using `mapcar' you are signaling that you are interested in the
> > return values of the argument function (and of course the resulting
> > list of them).
>
> > By using `mapc' you are signaling that the values returned by the
> > argument function are unimportant (only its side effects are
> > significant).
>
> How do you signal that the values returned by the argument
> function are unimportant, /and/ that you’re interested in the
> /original/ list instead?
By doing what you've done now. Neither `mapc' nor `mapcar' says
that, on its own.
If return values are not important in some code then `mapc`,
`dolist`, `while`, or any number of other imperative/procedural
approaches are fine to use, and are often preferable.
Looking briefly at the original code and your patches now, I am
reminded that `find-file*' needs to return the list of buffers
(or the single buffer, for that case). (As one user, for example,
I have code that expects the list of buffers to be returned.)
Your new patch does that, and it looks OK to me. I did not spend
much time looking things over, so I might be missing something,
but it looks like you are not changing the external behavior (good).
It is fine to optimize things, but function interfaces (signatures
and behavior as viewed by calling functions or by users) should
not be altered.
You might want to make sure that the list of buffers returned is
the same in all cases as what is returned today. The doc string
of `switch-to-buffer', for instance, seems to make a point of
saying that it returns the buffer switched to. It does not say
that it returns the buffer indicated by argument BUFFER-OR-NAME.
Dunno why, but it does.
I'm guessing that that can make a difference only if the intended
buffer cannot be switched to (an error is raised). And in that
case I'm guessing that the error raised would be handled
correctly by `find-file*'. (I have not checked.)
You might also want to check to be sure that the effect on what
`buffer-list' returns (after the calls to `switch-buffer*')
remains the same.
The calls to `switch-to-buffer*' here do not use non-nil NORECORD,
so that at least can be ignored. But maybe take a look, to make
sure. Some commands and other functions depend on the buffer
order in `buffer-list', so this too should not be altered by
your proposed change.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Sun, 03 Aug 2014 18:26:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 18175 <at> debbugs.gnu.org (full text, mbox):
>>>>> Drew Adams <drew.adams <at> oracle.com> writes:
[…]
> Your new patch does that, and it looks OK to me. I did not spend
> much time looking things over, so I might be missing something, but
> it looks like you are not changing the external behavior (good).
> It is fine to optimize things, but function interfaces (signatures
> and behavior as viewed by calling functions or by users) should not
> be altered.
I expect neither of these patches to change the external
behavior.
> You might want to make sure that the list of buffers returned is the
> same in all cases as what is returned today. The doc string of
> `switch-to-buffer', for instance, seems to make a point of saying
> that it returns the buffer switched to. It does not say that it
> returns the buffer indicated by argument BUFFER-OR-NAME. Dunno why,
> but it does.
I guess it’s because switch-to-buffer passes its argument
through window-normalize-buffer-to-switch-to, which passes it
through get-buffer in turn. The docstring for the latter reads:
… If BUFFER-OR-NAME is a buffer, return it as given.
And I see no reason for a buffer /name/ to ever appear in the
lists returned from find-file-noselect.
> I'm guessing that that can make a difference only if the intended
> buffer cannot be switched to (an error is raised). And in that case
> I'm guessing that the error raised would be handled correctly by
> `find-file*'. (I have not checked.)
As there’re no condition-case forms around switch-to-buffer
calls in find-file et al. – no such error will be handled in any
special way, and will just be signalled to the user as usual.
> You might also want to check to be sure that the effect on what
> `buffer-list' returns (after the calls to `switch-buffer*') remains
> the same.
AIUI, the buffer-list result depends solely on the order of
calls to switch-to-buffer, which is /not/ changed in any way.
> The calls to `switch-to-buffer*' here do not use non-nil NORECORD, so
> that at least can be ignored. But maybe take a look, to make sure.
> Some commands and other functions depend on the buffer order in
> `buffer-list', so this too should not be altered by your proposed
> change.
--
FSF associate member #7257 http://boycottsystemd.org/ … 3013 B6A0 230E 334A
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Sun, 03 Aug 2014 18:45:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 18175 <at> debbugs.gnu.org (full text, mbox):
> The docstring for the latter reads:
> … If BUFFER-OR-NAME is a buffer, return it as given.
> And I see no reason for a buffer /name/ to ever appear in the
> lists returned from find-file-noselect.
Yes, of course not. It needs to return a list of buffers. That's why I
wrote that we need to "make sure that the list of buffers returned is
the same in all cases as what is returned today." ^^^^^^^
> > I'm guessing that that can make a difference only if the intended
> > buffer cannot be switched to (an error is raised). And in that case
> > I'm guessing that the error raised would be handled correctly by
> > `find-file*'. (I have not checked.)
>
> As there’re no condition-case forms around switch-to-buffer
> calls in find-file et al. – no such error will be handled in any
> special way, and will just be signalled to the user as usual.
That all I meant by handled (treated) by `find-file*', not that
there is a `condition-case' with a handler. The point is that the
behavior should remain the same; that's all. If it does, fine.
> > You might also want to check to be sure that the effect on what
> > `buffer-list' returns (after the calls to `switch-buffer*') remains
> > the same.
>
> AIUI, the buffer-list result depends solely on the order of
> calls to switch-to-buffer, which is /not/ changed in any way.
>
> > The calls to `switch-to-buffer*' here do not use non-nil NORECORD, so
> > that at least can be ignored. But maybe take a look, to make sure.
> > Some commands and other functions depend on the buffer order in
> > `buffer-list', so this too should not be altered by your proposed
> > change.
Thanks for looking at and testing these things.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Wed, 06 Aug 2014 17:27:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 18175 <at> debbugs.gnu.org (full text, mbox):
> Given that switch-to-buffer returns its argument, /and/ given
> that mapc returns the sequence it’s given, I suggest that the
> (mapcar 'switch-to-buffer LIST) forms in lisp/files.el be
> replaced with (mapc 'switch-to-buffer LIST), – if only to avoid
> the unnecessary consing when the list is effectively copied in
> the mapcar case.
Thanks, I think it's indeed a valid/correct optimization, but I really
dislike relying on mapc's return value (it really should not return any
value at all).
In this case, the optimization doesn't seem worth the inconvenient of
having a very unusual code (relying on mapc's return value), since
those few cons cells we save are drowned in the noise of all the
code run by switch-to-buffer.
Stefan
Added tag(s) wontfix.
Request was from
Glenn Morris <rgm <at> gnu.org>
to
control <at> debbugs.gnu.org
.
(Thu, 07 Aug 2014 00:05:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Thu, 07 Aug 2014 19:16:02 GMT)
Full text and
rfc822 format available.
Message #28 received at 18175 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>>>>> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> Given that switch-to-buffer returns its argument, /and/ given that
>> mapc returns the sequence it’s given, I suggest that the (mapcar
>> 'switch-to-buffer LIST) forms in lisp/files.el be replaced with
>> (mapc 'switch-to-buffer LIST), – if only to avoid the unnecessary
>> consing when the list is effectively copied in the mapcar case.
> Thanks, I think it's indeed a valid/correct optimization, but I
> really dislike relying on mapc's return value (it really should not
> return any value at all).
I tend to disagree with that last part, – it seems like a common
idiom for a function (or, generally, – a /form/; setq does that,
for one thing) that’s used “solely” for its side-effects to
return its “primary” argument, thus allowing for easy
“chaining”, like:
(foo (bar 42 (baz data)))
Instead of:
(progn
(foo data)
(bar 42 data)
(baz data))
Or something even more crude, like:
(mapc (lambda (fn)
(if (consp fn)
(apply (car fn) `(,@(cdr fn) ,data))
(funcall fn data)))
'(foo (bar 42) baz))
Surely, ‘mapc’ fits that pattern pretty well; cf.:
(foo (bar 42 (baz lst)))
(foo (mapc 'qux (baz lst)))
> In this case, the optimization doesn't seem worth the inconvenient of
> having a very unusual code (relying on mapc's return value), since
> those few cons cells we save are drowned in the noise of all the code
> run by switch-to-buffer.
Yes. However, I believe that the last two hunks of the one
another variant of the diff (MIMEd) actually make the intent to
return the reverse of the list returned by find-file-noselect
/clearer,/ – although at the expense of adding one extra LoC in
each case.
--
FSF associate member #7257 http://boycottsystemd.org/ … 3013 B6A0 230E 334A
[Message part 2 (text/x-diff, inline)]
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1428,7 +1428,7 @@ automatically choosing a major mode, use \\[find-file-literally]."
(confirm-nonexistent-file-or-buffer)))
(let ((value (find-file-noselect filename nil nil wildcards)))
(if (listp value)
- (mapcar 'switch-to-buffer (nreverse value))
+ (mapc 'switch-to-buffer (nreverse value))
(switch-to-buffer value))))
(defun find-file-other-window (filename &optional wildcards)
@@ -1450,8 +1450,9 @@ expand wildcards (if any) and visit multiple files."
(if (listp value)
(progn
(setq value (nreverse value))
- (cons (switch-to-buffer-other-window (car value))
- (mapcar 'switch-to-buffer (cdr value))))
+ (switch-to-buffer-other-window (car value))
+ (mapc 'switch-to-buffer (cdr value))
+ value)
(switch-to-buffer-other-window value))))
(defun find-file-other-frame (filename &optional wildcards)
@@ -1473,8 +1474,9 @@ expand wildcards (if any) and visit multiple files."
(if (listp value)
(progn
(setq value (nreverse value))
- (cons (switch-to-buffer-other-frame (car value))
- (mapcar 'switch-to-buffer (cdr value))))
+ (switch-to-buffer-other-frame (car value))
+ (mapc 'switch-to-buffer (cdr value))
+ value)
(switch-to-buffer-other-frame value))))
(defun find-file-existing (filename)
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Thu, 07 Aug 2014 19:30:03 GMT)
Full text and
rfc822 format available.
Message #31 received at 18175 <at> debbugs.gnu.org (full text, mbox):
> I tend to disagree with that last part, – it seems like a common
> idiom for a function (or, generally, – a /form/; setq does that,
> for one thing) that’s used “solely” for its side-effects to
> return its “primary” argument, thus allowing for easy
Yes, it's common, but I strongly dislike it.
It's used often enough for `setq' that I consider it to be an exception.
In the case of `mapc' OTOH, I'm pretty sure 99.9% of Elisp coders have no
idea what is the return value of `mapc', so using this return value is
a kind of obfuscation.
> Yes. However, I believe that the last two hunks of the one
> another variant of the diff (MIMEd) actually make the intent to
> return the reverse of the list returned by find-file-noselect
> /clearer,/ – although at the expense of adding one extra LoC in
> each case.
Agreed, and neither relies on the return value of `mapc', so those two
hunks are indeed good. Please install them.
Stefan
Removed tag(s) wontfix.
Request was from
Glenn Morris <rgm <at> gnu.org>
to
control <at> debbugs.gnu.org
.
(Thu, 07 Aug 2014 19:37:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#18175
; Package
emacs
.
(Fri, 21 Nov 2014 10:59:02 GMT)
Full text and
rfc822 format available.
Message #36 received at 18175 <at> debbugs.gnu.org (full text, mbox):
Stefan, you’ve previously commented on several of the bug
reports I’ve suggested patches for. Could you please revisit
these patches and either commit or explicitly reject them?
(Any suggestion on how, if at all, do I improve them in the
latter case will be appreciated.)
The bug reports in question are as follows.
TIA.
http://debbugs.gnu.org/18175 Use (mapc 'switch-to-buffer …) in
files.el (was: mapcar.)
http://debbugs.gnu.org/18246 Use inhibit-point-motion-hooks
(in addition to inhibit-read-only) in enriched-encode.
http://debbugs.gnu.org/18824 New diff-check-labels function
(split off diff-no-select)
http://debbugs.gnu.org/18850 Use diff-check-labels and the --label
option to diff (was: -L – deprecated) in smerge-mode.el
--
FSF associate member #7257 np. По полям, за рекой — Иллет … B6A0 230E 334A
Added tag(s) patch.
Request was from
Ivan Shmakov <ivan <at> siamics.net>
to
control <at> debbugs.gnu.org
.
(Fri, 26 Dec 2014 17:51:02 GMT)
Full text and
rfc822 format available.
Reply sent
to
Ivan Shmakov <ivan <at> siamics.net>
:
You have taken responsibility.
(Fri, 23 Jan 2015 15:28:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Ivan Shmakov <ivan <at> siamics.net>
:
bug acknowledged by developer.
(Fri, 23 Jan 2015 15:28:03 GMT)
Full text and
rfc822 format available.
Message #43 received at 18175-done <at> debbugs.gnu.org (full text, mbox):
Version: 25.1
>>>>> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
[…]
>> However, I believe that the last two hunks of the one another
>> variant of the diff (MIMEd) actually make the intent to return the
>> reverse of the list returned by find-file-noselect /clearer,/ –
>> although at the expense of adding one extra LoC in each case.
> Agreed, and neither relies on the return value of `mapc', so those
> two hunks are indeed good. Please install them.
Done; closing.
commit 3e824b05af3a75768a61001fad68e2340af810eb
CommitDate: 2015-01-17 19:33:08 +0000
Avoid mapcar in two cases in files.el.
* lisp/files.el (find-file-other-window, find-file-other-frame):
Use mapc instead of mapcar.
Fixes: debbugs:18175
--
FSF associate member #7257 np. Chemical Wedding – Bruce Dickinson 230E 334A
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Sat, 21 Feb 2015 12:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 9 years and 58 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.