bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 1257 bytes --]

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)

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Drew Adams]

> 	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.

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

>>>>> Drew Adams <drew.adams@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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 1752 bytes --]

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)

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Drew Adams]

>  > 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.

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Ivan Shmakov]

>>>>> Drew Adams <drew.adams@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

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Drew Adams]

> 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.

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Stefan Monnier]

> 	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

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Ivan Shmakov]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

>>>>> Stefan Monnier <monnier@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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 1345 bytes --]

--- 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)

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Stefan Monnier]

> 	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

bug#18175: files.el: use mapc in (mapcar 'switch-to-buffer ...)

[Ivan Shmakov]

Version: 25.1

>>>>> Stefan Monnier <monnier@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