Re: master a6b5985: Avoid duplicated character classes in rx

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 3, 2019 at 3:23 PM Mattias Engdegård <mattiase@savannah.gnu.org>
wrote:

>              ((and (symbolp arg)
>                    (let ((class (cdr (assq arg rx--char-classes))))
> -                    (and class (push class classes)))))
> +                    (and class
> +                         (or (memq class classes)
> +                             (push class classes))))))

This (which is a branch of a `cond') relies in the fact that (push ELEMENT
LISTNAME)
returns the new LISTNAME.

Which isn't really documented. It's sort-of-documented because push's
docstring
says that it is "morally equivalent to (setf place (cons newelt place)",
and the
elisp manual says that it is "equivalent to (setq ...)" or that it "does the
equivalent of (setf ...)".

Shouldn't we say it in its docstring?

(BTW, push's args are called ELEMENT and LISTNAME in the manual, NEWELT and
PLACE
in the code. Frankly, I think they should be ELEMENT and PLACE in both
cases.)

[-- Attachment #2: Type: text/html, Size: 1954 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

>>              ((and (symbolp arg)
>>                    (let ((class (cdr (assq arg rx--char-classes))))
>> -                    (and class (push class classes)))))
>> +                    (and class
>> +                         (or (memq class classes)
>> +                             (push class classes))))))
>
> This (which is a branch of a `cond') relies in the fact that (push ELEMENT
> LISTNAME)
> returns the new LISTNAME.
>
> Which isn't really documented. It's sort-of-documented because push's
> docstring
> says that it is "morally equivalent to (setf place (cons newelt place)",
> and the
> elisp manual says that it is "equivalent to (setq ...)" or that it "does the
> equivalent of (setf ...)".
>
> Shouldn't we say it in its docstring?

I'd rather fix the code not to rely on the return value.
E.g.

                 ((and (symbolp arg)
                       (let ((class (cdr (assq arg rx--char-classes))))
    -                    (and class (push class classes)))))
    +                    (and class
    +                         (progn (cl-pushnew class classes) t))))


-- Stefan

Re: master a6b5985: Avoid duplicated character classes in rx

[Mattias Engdegård]

3 dec. 2019 kl. 16.26 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> I'd rather fix the code not to rely on the return value.

I'm sure a lot more code relies on the return value of 'push'.
It's also well-defined in Common Lisp. Why not just document it?

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

>> I'd rather fix the code not to rely on the return value.
> I'm sure a lot more code relies on the return value of 'push'.

Probably, but I still think it's bad practice to use the return value of
an operation which is fundamentally a side-effect.


        Stefan

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 3, 2019 at 5:01 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Probably, but I still think it's bad practice to use the return value of
> an operation which is fundamentally a side-effect.

So you've never written

(if (setq var value) ...)

?  ;-)

[-- Attachment #2: Type: text/html, Size: 423 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 3 Dec 2019 17:06:16 +0100
> Cc: Mattias Engdegård <mattiase@acm.org>,
>  Emacs developers <emacs-devel@gnu.org>
> 
> So you've never written
> 
> (if (setq var value) ...)
> 
> ?  ;-)

The return value of setq _is_ documented.

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 3, 2019 at 6:37 PM Eli Zaretskii <eliz@gnu.org> wrote:

> The return value of setq _is_ documented.

Of course. But certainly, it is "the return value of an operation which is
fundamentally a side-effect", to quote Stefan. So every time he writes code
like that, some poor kitten is sacrificed to Cthulhu.

[-- Attachment #2: Type: text/html, Size: 538 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

> So every time he writes code like that, some poor kitten is sacrificed
> to Cthulhu.

Now I feel even more guilty!


        Stefan

RE: master a6b5985: Avoid duplicated character classes in rx

[Drew Adams]

> > So you've never written
> > (if (setq var value) ...)
> > ?  ;-)
> 
> The return value of setq _is_ documented.

Likewise `setf'.  Which is why the return
value of `push' is _indirectly_ documented.

That indirection already commits to support
of the return value (of `push').  We should
specify it explicitly.

As does Common Lisp (even though it too
refers to `setf' in the doc).  There's no
reason not to say clearly and directly what
`push' returns.

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

>> Probably, but I still think it's bad practice to use the return value of
>> an operation which is fundamentally a side-effect.
> So you've never written
> (if (setq var value) ...)

Guilty as charged.  But every time I do so I feel a bit dirty (and not
just because I feel dirty every time I use `setq`).
And I have often regretted writing such code.

To my defense, `setq` is the only such exception I can think of.


        Stefan

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 3, 2019 at 6:40 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Guilty as charged.

As we all are. Secretly or openly, all of us have done it occasionally.

> And I have often regretted writing such code.

;-)

> To my defense, `setq` is the only such exception I can think of.

I have sinned with `cl-incf' a few times, myself.

[-- Attachment #2: Type: text/html, Size: 509 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

>> To my defense, `setq` is the only such exception I can think of.
> I have sinned with `cl-incf' a few times, myself.

Now *this* is sick!
Your karma credit will not go back up until you atone for that,


        Stefan

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 3, 2019 at 7:36 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Now *this* is sick!

I know.

> Your karma credit will not go back up until you atone for that,

I was for a long while a Perl / C++ programmer. My karma credit is chthonic.

I try to atone with Lisp and Ada, but life is too short.

[-- Attachment #2: Type: text/html, Size: 498 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > Probably, but I still think it's bad practice to use the return value of
  > > an operation which is fundamentally a side-effect.

  > So you've never written

  > (if (setq var value) ...)

It is clearer to write

  (setq var value)
  (if var ...)

-- 
Dr Richard Stallman
Founder, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Wed, Dec 4, 2019 at 5:36 AM Richard Stallman <rms@gnu.org> wrote:

> It is clearer to write
>
>   (setq var value)
>   (if var ...)

Sure.

I think I've used (if (setq...) only in cases where it avoided an
indentation level
(having to add `progn' or somesuch) in code already overstuffed.

[-- Attachment #2: Type: text/html, Size: 521 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Michael Welsh Duggan]

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

>>> I'd rather fix the code not to rely on the return value.
>> I'm sure a lot more code relies on the return value of 'push'.
>
> Probably, but I still think it's bad practice to use the return value of
> an operation which is fundamentally a side-effect.

But isn't that standard practice?  From the elisp manual, for example:
(setq x (nreverse x))

-- 
Michael Welsh Duggan
(mwd@cert.org)

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

>>>> I'd rather fix the code not to rely on the return value.
>>> I'm sure a lot more code relies on the return value of 'push'.
>> Probably, but I still think it's bad practice to use the return value of
>> an operation which is fundamentally a side-effect.
> But isn't that standard practice?  From the elisp manual, for example:
> (setq x (nreverse x))

I don't think `nreverse` is "fundamentally a side-effect": its return
value is of critical importance.

Similarly return values which correspond to side-information about how
the side-effect was done (e.g. status or error codes) aren't subject to
the above "rule".


        Stefan

Re: master a6b5985: Avoid duplicated character classes in rx

[Mattias Engdegård]

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

> Probably, but I still think it's bad practice to use the return value of
> an operation which is fundamentally a side-effect.

Very well, it has now been wrapped in (progn ... t).

Juanma Barranquero <lekktu@gmail.com>:

> So you've never written
> 
> (if (setq var value) ...)

Not so much, but I find it harder to avoid

  (while (setq v e) ...)

lacking tail-recursive loops.

> Thinking about converting it to cl-pushnew is what made me look closer at this code in
> the first place ;-)

I prefer not using cl in rx.el at all for messy bootstrap reasons. (cl-pushnew is a macro, but I'd rather not tempt fate.)

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 1045 bytes --]

After looking at, literally, about 4,000 uses of `push' in our sources,
I've found 34 that rely on its return code. (I'm not claiming these are
all there is, just the ones I detected in what was, at most, a mechanical
and, frankly, quite tedious task, though hopefully not a sisyphean one.)

That's about a 0.08% of misuse. Don't know if that's an argument for,
or against, documenting push's return value.

But, assuming we're not going to document it, the following patch (with
appropriate ChangeLog entries, to be written) should be committed.

Changes are mostly mechanical, though in a few cases I've changed
slightly the code to better reflect the author's intention. For example,
there's one case where the FUNCTION arg of a mapcar was just a call to
push, but in fact, the whole return value of that mapcar wasn't used;
so I've changed the mapcar to mapc.

What I mean is that this isn't, per se, a destabilizing change, but
there are some subtleties. Eli, do I install it right now, or do you
prefer to wait after the 27 branch is cut?

[-- Attachment #1.2: Type: text/html, Size: 1629 bytes --]

[-- Attachment #2: push.patch --]
[-- Type: application/octet-stream, Size: 20101 bytes --]

diff --git i/lisp/emacs-lisp/generator.el w/lisp/emacs-lisp/generator.el
index 9dba87eaeb..6ad3ffb72f 100644
--- i/lisp/emacs-lisp/generator.el
+++ w/lisp/emacs-lisp/generator.el
@@ -161,8 +161,9 @@ cps--add-state
     state))
 
 (defun cps--add-binding (original-name)
-  (car (push (cps--gensym (format "cps-binding-%s-" original-name))
-             cps--bindings)))
+  (push (cps--gensym (format "cps-binding-%s-" original-name))
+        cps--bindings)
+  (car cps--bindings))
 
 (defun cps--find-special-form-handler (form)
   (let* ((handler-name (format "cps--transform-%s" (car-safe form)))
diff --git i/lisp/emacs-lisp/rx.el w/lisp/emacs-lisp/rx.el
index 0dc6e19866..41e9bf666f 100644
--- i/lisp/emacs-lisp/rx.el
+++ w/lisp/emacs-lisp/rx.el
@@ -1233,9 +1233,10 @@ rx--pcase-transform
   (pcase rx
     (`(let ,name . ,body)
      (let* ((index (length (memq name rx--pcase-vars)))
-            (i (if (zerop index)
-                   (length (push name rx--pcase-vars))
-                 index)))
+            (i (if (> index 0)
+                   index
+                 (push name rx--pcase-vars)
+                 (length rx--pcase-vars))))
        `(group-n ,i ,(rx--pcase-transform (cons 'seq body)))))
     ((and `(backref ,ref)
           (guard (symbolp ref)))
diff --git i/lisp/emacs-lisp/seq.el w/lisp/emacs-lisp/seq.el
index 7e5e6f3b16..cd807b3c28 100644
--- i/lisp/emacs-lisp/seq.el
+++ w/lisp/emacs-lisp/seq.el
@@ -472,7 +472,7 @@ seq-group-by
      (let* ((key (funcall function elt))
             (cell (assoc key acc)))
        (if cell
-           (setcdr cell (push elt (cdr cell)))
+           (setcdr cell (cons elt (cdr cell)))
          (push (list key elt) acc))
        acc))
    (seq-reverse sequence)
diff --git i/lisp/erc/erc-imenu.el w/lisp/erc/erc-imenu.el
index 3e1455ae5b..36ec0bc27d 100644
--- i/lisp/erc/erc-imenu.el
+++ w/lisp/erc/erc-imenu.el
@@ -89,21 +89,21 @@ erc-create-imenu-index
 	    (push (cons notice-text pos) notice-alist)
 	    (or
 	     (when (string-match "^\\(.*\\) has joined channel" notice-text)
-	       (push (cons (match-string 1 notice-text) pos) join-alist))
+	       (push (cons (match-string 1 notice-text) pos) join-alist) t)
 	     (when (string-match "^\\(.+\\) has left channel" notice-text)
-	       (push (cons (match-string 1 notice-text) pos) left-alist))
+	       (push (cons (match-string 1 notice-text) pos) left-alist) t)
 	     (when (string-match "^\\(.+\\) has quit\\(.*\\)$" notice-text)
 	       (push (cons (concat (match-string 1 notice-text)
 				   (match-string 2 notice-text))
 			   (point))
-		     quit-alist))
+		     quit-alist) t)
 	     (when (string-match
 		    "^\\(\\S-+\\) (.+) has changed mode for \\S-+ to \\(.*\\)$"
 		    notice-text)
 	       (push (cons (concat (match-string 1 notice-text) ": "
 				   (match-string 2 notice-text))
 			   (point))
-		     mode-change-alist))
+		     mode-change-alist) t)
 	     (when (string-match
 		    "^\\(\\S-+\\) (.+) has set the topic for \\S-+: \\(.*\\)$"
 		    notice-text)
diff --git i/lisp/files.el w/lisp/files.el
index a384e7136e..33870c2b6b 100644
--- i/lisp/files.el
+++ w/lisp/files.el
@@ -4007,7 +4007,8 @@ dir-locals-collect-mode-variables
       (if (and slot (not (memq variable '(mode eval))))
 	  (setcdr slot value)
 	;; Need a new cons in case we setcdr later.
-	(push (cons variable value) variables)))))
+	(push (cons variable value) variables)
+	variables))))
 
 (defun dir-locals-collect-variables (class-variables root variables)
   "Collect entries from CLASS-VARIABLES into VARIABLES.
diff --git i/lisp/gnus/gnus-art.el w/lisp/gnus/gnus-art.el
index cfb185b3d1..9eac056ede 100644
--- i/lisp/gnus/gnus-art.el
+++ w/lisp/gnus/gnus-art.el
@@ -3964,7 +3964,8 @@ gnus-read-save-file-name
 				   gnus-article-save-directory
 				   (car split-name))
 				  gnus-article-save-directory)))
-			 (car (push result file-name-history)))))))
+			 (push result file-name-history)
+			 result)))))
 	       ;; Create the directory.
 	       (gnus-make-directory (file-name-directory file))
 	       ;; If we have read a directory, we append the default file name.
diff --git i/lisp/gnus/gnus-group.el w/lisp/gnus/gnus-group.el
index 5d68163ff3..18e93de820 100644
--- i/lisp/gnus/gnus-group.el
+++ w/lisp/gnus/gnus-group.el
@@ -1968,10 +1968,10 @@ gnus-group-process-prefix
       (save-excursion
 	(goto-char (min (point) (mark)))
 	(while
-	    (and
-	     (push (gnus-group-group-name) groups)
-	     (zerop (gnus-group-next-group 1))
-	     (< (point) max)))
+	    (progn
+	      (push (gnus-group-group-name) groups)
+	      (and (zerop (gnus-group-next-group 1))
+		   (< (point) max))))
 	(nreverse groups))))
    (gnus-group-marked
     ;; No prefix, but a list of marked articles.
diff --git i/lisp/gnus/gnus-picon.el w/lisp/gnus/gnus-picon.el
index 18b46a1c12..8246307aff 100644
--- i/lisp/gnus/gnus-picon.el
+++ w/lisp/gnus/gnus-picon.el
@@ -158,10 +158,11 @@ gnus-picon-insert-glyph
 
 (defun gnus-picon-create-glyph (file)
   (or (cdr (assoc file gnus-picon-glyph-alist))
-      (cdar (push (cons file (apply 'gnus-create-image
-				    file nil nil
-				    gnus-picon-properties))
-		  gnus-picon-glyph-alist))))
+      (let ((glyph (apply 'gnus-create-image
+			  file nil nil
+			  gnus-picon-properties)))
+	(push (cons file glyph) gnus-picon-glyph-alist)
+	glyph)))
 
 ;;; Functions that does picon transformations:
 
diff --git i/lisp/gnus/gnus-sum.el w/lisp/gnus/gnus-sum.el
index d62c063436..88cf025cd7 100644
--- i/lisp/gnus/gnus-sum.el
+++ w/lisp/gnus/gnus-sum.el
@@ -6716,8 +6716,10 @@ gnus-summary-work-articles
 	(save-excursion
 	  (while
 	      (and (> n 0)
-		   (push (setq article (gnus-summary-article-number))
-			 articles)
+		   (progn
+		     (push (setq article (gnus-summary-article-number))
+			   articles)
+		     t)
 		   (if backward
 		       (gnus-summary-find-prev nil article)
 		     (gnus-summary-find-next nil article)))
@@ -6732,7 +6734,9 @@ gnus-summary-work-articles
 	  (goto-char (min (point) (mark)))
 	  (while
 	      (and
-	       (push (setq article (gnus-summary-article-number)) articles)
+	       (progn
+		 (push (setq article (gnus-summary-article-number)) articles)
+		 t)
 	       (gnus-summary-find-next nil article)
 	       (< (point) max)))
 	  (nreverse articles))))
diff --git i/lisp/gnus/gnus-uu.el w/lisp/gnus/gnus-uu.el
index 253ee24f32..beb4314f23 100644
--- i/lisp/gnus/gnus-uu.el
+++ w/lisp/gnus/gnus-uu.el
@@ -1153,8 +1153,9 @@ gnus-uu-get-list-of-articles
 	    (n (abs n)))
 	(save-excursion
 	  (while (and (> n 0)
-		      (push (gnus-summary-article-number)
-			    articles)
+		      (progn
+			(push (gnus-summary-article-number) articles)
+			t)
 		      (gnus-summary-search-forward nil nil backward))
 	    (setq n (1- n))))
 	(nreverse articles)))
diff --git i/lisp/gnus/gnus.el w/lisp/gnus/gnus.el
index 23643cc6c7..b6dc9efab4 100644
--- i/lisp/gnus/gnus.el
+++ w/lisp/gnus/gnus.el
@@ -655,7 +655,9 @@ gnus-buffers
 (defun gnus-get-buffer-create (name)
   "Do the same as `get-buffer-create', but store the created buffer."
   (or (get-buffer name)
-      (car (push (get-buffer-create name) gnus-buffers))))
+      (let ((buffer (get-buffer-create name)))
+        (push buffer gnus-buffers)
+        buffer)))
 
 (defun gnus-add-buffer ()
   "Add the current buffer to the list of Gnus buffers."
diff --git i/lisp/gnus/nntp.el w/lisp/gnus/nntp.el
index 3ddd53e46c..d23998b627 100644
--- i/lisp/gnus/nntp.el
+++ w/lisp/gnus/nntp.el
@@ -1299,7 +1299,8 @@ nntp-open-connection
       (if (and (nntp-wait-for process "^2.*\n" buffer nil t)
 	       (memq (process-status process) '(open run)))
 	  (prog1
-	      (caar (push (list process buffer nil) nntp-connection-alist))
+	      process
+	    (push (list process buffer nil) nntp-connection-alist)
 	    (push process nntp-connection-list)
 	    (with-current-buffer pbuffer
 	      (nntp-read-server-type)
diff --git i/lisp/ibuf-ext.el w/lisp/ibuf-ext.el
index 12930fc0a6..76cca3d9b8 100644
--- i/lisp/ibuf-ext.el
+++ w/lisp/ibuf-ext.el
@@ -1027,10 +1027,11 @@ ibuffer-pop-filter
 
 (defun ibuffer-push-filter (filter-specification)
   "Add FILTER-SPECIFICATION to `ibuffer-filtering-qualifiers'.
-If FILTER-SPECIFICATION is already in the list then return nil.  Otherwise,
-return the updated list."
+If FILTER-SPECIFICATION is already in the list then return nil.
+Otherwise, return the updated list."
   (unless (member filter-specification ibuffer-filtering-qualifiers)
-    (push filter-specification ibuffer-filtering-qualifiers)))
+    (push filter-specification ibuffer-filtering-qualifiers)
+    ibuffer-filtering-qualifiers))
 
 ;;;###autoload
 (defun ibuffer-decompose-filter ()
diff --git i/lisp/jka-cmpr-hook.el w/lisp/jka-cmpr-hook.el
index 3aa84f45b0..63415e7d9d 100644
--- i/lisp/jka-cmpr-hook.el
+++ w/lisp/jka-cmpr-hook.el
@@ -141,17 +141,17 @@ jka-compr-install
       (push elt file-coding-system-alist)
       (push elt jka-compr-added-to-file-coding-system-alist))
 
-    (and (jka-compr-info-strip-extension x)
-         ;; Make entries in auto-mode-alist so that modes
-         ;; are chosen right according to the file names
-         ;; sans `.gz'.
-         (push (list (jka-compr-info-regexp x) nil 'jka-compr) auto-mode-alist)
-         ;; Also add these regexps to inhibit-local-variables-suffixes,
-         ;; so that a -*- line in the first file of a compressed tar file,
-         ;; or a Local Variables section in a member file at the end of
-         ;; the tar file don't override tar-mode.
-         (push (jka-compr-info-regexp x)
-               inhibit-local-variables-suffixes)))
+    (when (jka-compr-info-strip-extension x)
+      ;; Make entries in auto-mode-alist so that modes
+      ;; are chosen right according to the file names
+      ;; sans `.gz'.
+      (push (list (jka-compr-info-regexp x) nil 'jka-compr) auto-mode-alist)
+      ;; Also add these regexps to inhibit-local-variables-suffixes,
+      ;; so that a -*- line in the first file of a compressed tar file,
+      ;; or a Local Variables section in a member file at the end of
+      ;; the tar file don't override tar-mode.
+      (push (jka-compr-info-regexp x)
+            inhibit-local-variables-suffixes)))
   (setq auto-mode-alist
 	(append auto-mode-alist jka-compr-mode-alist-additions))
 
diff --git i/lisp/mh-e/mh-search.el w/lisp/mh-e/mh-search.el
index 97f1fddcd0..8106d270a2 100644
--- i/lisp/mh-e/mh-search.el
+++ w/lisp/mh-e/mh-search.el
@@ -750,10 +750,11 @@ mh-index-add-implicit-ops
       (cond ((or (equal current ")") (equal current "and") (equal current "or"))
              (setq literal-seen nil)
              (push current result))
-            ((and literal-seen
-                  (push "and" result)
-                  (setq literal-seen nil)
-                  nil))
+            ((when literal-seen
+               (push "and" result)
+               (setq literal-seen nil)
+               ;; falls through
+               nil))
             (t
              (push current result)
              (unless (or (equal current "(") (equal current "not"))
diff --git i/lisp/net/tramp.el w/lisp/net/tramp.el
index e344990f7f..f8a0ae9878 100644
--- i/lisp/net/tramp.el
+++ w/lisp/net/tramp.el
@@ -3271,7 +3271,7 @@ tramp-handle-file-name-completion
 	    (string-match-p
 	     (concat (regexp-opt completion-ignored-extensions 'paren) "$") x)
 	    ;; We remember the hit.
-	    (push x hits-ignored-extensions))))))
+	    (progn (push x hits-ignored-extensions) t))))))
      ;; No match.  So we try again for ignored files.
      (try-completion filename hits-ignored-extensions))))
 
diff --git i/lisp/org/ob-core.el w/lisp/org/ob-core.el
index f877ff51bf..c28d5c6ab4 100644
--- i/lisp/org/ob-core.el
+++ w/lisp/org/ob-core.el
@@ -1569,9 +1569,9 @@ org-babel-parse-multiple-vars
   (let (results)
     (mapc (lambda (pair)
 	    (if (eq (car pair) :var)
-		(mapcar (lambda (v) (push (cons :var (org-trim v)) results))
-			(org-babel-join-splits-near-ch
-			 61 (org-babel-balanced-split (cdr pair) 32)))
+		(mapc (lambda (v) (push (cons :var (org-trim v)) results))
+		      (org-babel-join-splits-near-ch
+		       61 (org-babel-balanced-split (cdr pair) 32)))
 	      (push pair results)))
 	  header-arguments)
     (nreverse results)))
diff --git i/lisp/org/org-list.el w/lisp/org/org-list.el
index c4aef32fc0..7755aa0750 100644
--- i/lisp/org/org-list.el
+++ w/lisp/org/org-list.el
@@ -691,12 +691,13 @@ org-list-struct
 	      ;; At downward limit: this is de facto the end of the
 	      ;; list.  Save point as an ending position, and jump to
 	      ;; part 3.
-	      (throw 'exit
-		     (push (cons 0 (funcall end-before-blank)) end-lst-2)))
+	      (push (cons 0 (funcall end-before-blank)) end-lst-2)
+	      (throw 'exit end-lst-2))
 	     ;; Looking at a list ending regexp.  Save point as an
 	     ;; ending position and jump to part 3.
 	     ((looking-at org-list-end-re)
-	      (throw 'exit (push (cons 0 (point)) end-lst-2)))
+	      (push (cons 0 (point)) end-lst-2)
+	      (throw 'exit end-lst-2))
 	     ((looking-at item-re)
 	      ;; Point is at an item.  Add data to ITM-LST-2. It may
 	      ;; also end a previous item, so save it in END-LST-2.
@@ -712,8 +713,8 @@ org-list-struct
 	     ;; over: store point as an ending position and jump to
 	     ;; part 3.
 	     ((<= ind (cdr beg-cell))
-	      (throw 'exit
-		     (push (cons 0 (funcall end-before-blank)) end-lst-2)))
+	      (push (cons 0 (funcall end-before-blank)) end-lst-2)
+	      (throw 'exit end-lst-2))
 	     ;; Else, if ind is lesser or equal than previous item's,
 	     ;; this is an ending position: store it.  In any case,
 	     ;; skip block or drawer at point, and move to next line.
diff --git i/lisp/org/ox.el w/lisp/org/ox.el
index 5b4134ecca..58f97ea49d 100644
--- i/lisp/org/ox.el
+++ w/lisp/org/ox.el
@@ -5088,8 +5088,8 @@ org-export-table-cell-borders
 	  (cond ((eq (org-element-property :type row) 'rule)
 		 (setq rule-flag t))
 		((not (org-export-table-row-is-special-p row info))
-		 (if rule-flag (throw 'exit (push 'above borders))
-		   (throw 'exit nil)))))
+		 (throw 'exit
+		   (when rule-flag (push 'above borders) borders)))))
 	;; No rule above, or rule found starts the table (ignoring any
 	;; special row): TABLE-CELL is at the top of the table.
 	(when rule-flag (push 'above borders))
@@ -5103,8 +5103,8 @@ org-export-table-cell-borders
 	  (cond ((eq (org-element-property :type row) 'rule)
 		 (setq rule-flag t))
 		((not (org-export-table-row-is-special-p row info))
-		 (if rule-flag (throw 'exit (push 'below borders))
-		   (throw 'exit nil)))))
+		 (throw 'exit
+		   (when rule-flag (push 'below borders) borders)))))
 	;; No rule below, or rule found ends the table (modulo some
 	;; special row): TABLE-CELL is at the bottom of the table.
 	(when rule-flag (push 'below borders))
diff --git i/lisp/play/decipher.el w/lisp/play/decipher.el
index 52683afeb3..e07c7f4d95 100644
--- i/lisp/play/decipher.el
+++ w/lisp/play/decipher.el
@@ -820,8 +820,10 @@ decipher--analyze
   ;;     of the corresponding characters.
   (setq decipher--digram (format "%c%c" decipher--prev-char decipher-char))
   (cl-incf (cdr (or (assoc decipher--digram decipher--digram-list)
-                 (car (push (cons decipher--digram 0)
-                            decipher--digram-list)))))
+                    (progn
+                      (push (cons decipher--digram 0)
+                            decipher--digram-list)
+                      (car decipher--digram-list)))))
   (and (>= decipher--prev-char ?A)
        (cl-incf (aref (aref decipher--before (- decipher--prev-char ?A))
                    (if (equal decipher-char ?\s)
diff --git i/lisp/recentf.el w/lisp/recentf.el
index 0828bfc2ba..516ae9a8d6 100644
--- i/lisp/recentf.el
+++ w/lisp/recentf.el
@@ -854,7 +854,9 @@ recentf-arrange-by-rule
         (if (not (stringp (car menu)))
             (push elt others)
           (setq menu (or (assoc (car menu) menus)
-                         (car (push (list (car menu)) menus))))
+                         (progn
+                           (push (list (car menu)) menus)
+                           (car menus))))
           (recentf-set-menu-element-value
            menu (cons elt (recentf-menu-element-value menu)))))
       ;; Finalize each sub-menu:
diff --git i/lisp/textmodes/reftex-parse.el w/lisp/textmodes/reftex-parse.el
index eb8446f4c4..420ec8fa2b 100644
--- i/lisp/textmodes/reftex-parse.el
+++ w/lisp/textmodes/reftex-parse.el
@@ -140,7 +140,9 @@ reftex-do-parse
            (bof2 (assq 'bof (cdr bof1)))
            (is-multi (not (not (and bof1 bof2))))
            (entry (or (assq 'is-multi docstruct)
-                      (car (push (list 'is-multi is-multi) docstruct)))))
+                      (progn
+                        (push (list 'is-multi is-multi) docstruct)
+                        (car docstruct)))))
       (setcdr entry (cons is-multi nil)))
     (and reftex--index-tags
          (setq reftex--index-tags (sort reftex--index-tags 'string<)))
diff --git i/lisp/vc/vc-svn.el w/lisp/vc/vc-svn.el
index a2081e1ab9..28715e8a31 100644
--- i/lisp/vc/vc-svn.el
+++ w/lisp/vc/vc-svn.el
@@ -359,10 +359,10 @@ vc-svn-ignore
   (let* ((path (directory-file-name (expand-file-name file directory)))
          (directory (file-name-directory path))
          (file (file-name-nondirectory path))
-         (ignores (vc-svn-ignore-completion-table directory))
-         (ignores (if remove
-                      (delete file ignores)
-                    (push file ignores))))
+         (ignores (vc-svn-ignore-completion-table directory)))
+    (if remove
+        (setq ignores (delete file ignores))
+      (push file ignores))
     (vc-svn-command nil 0 nil nil "propset" "svn:ignore"
                     (mapconcat #'identity ignores "\n")
                     directory)))
diff --git i/lisp/winner.el w/lisp/winner.el
index a97bfbde9d..0bc122fd6e 100644
--- i/lisp/winner.el
+++ w/lisp/winner.el
@@ -236,9 +236,11 @@ winner-make-point-alist
              for win in (window-list nil 0)
              for entry =
              (or (assq (window-buffer win) alist)
-                 (car (push (list (set-buffer (window-buffer win))
-                                  (cons (mark t) (winner-active-region)))
-                            alist)))
+                 (progn
+                   (push (list (set-buffer (window-buffer win))
+                               (cons (mark t) (winner-active-region)))
+                         alist)
+                   (car alist)))
              do (push (cons win (window-point win))
                       (cddr entry))
              finally return alist)))
diff --git i/test/lisp/emacs-lisp/cl-macs-tests.el w/test/lisp/emacs-lisp/cl-macs-tests.el
index 8523044714..fe48d06dce 100644
--- i/test/lisp/emacs-lisp/cl-macs-tests.el
+++ w/test/lisp/emacs-lisp/cl-macs-tests.el
@@ -565,21 +565,22 @@ cl-macs-loop-conditional-step-clauses
 
   (should (equal (cl-loop with result
                           for x below 3
-                          for _y = (progn (push x result))
+                          for _y = (progn (push x result) result)
                           finally return result)
                  '(2 1 0)))
 
   ;; this nonintuitive result is replicated by clisp
   (should (equal (cl-loop with result
                           for x below 3
-                          and y = (progn (push x result))
+                          and y = (progn (push x result) result)
                           finally return result)
                  '(2 1 0 0)))
 
   ;; this nonintuitive result is replicated by clisp
   (should (equal (cl-loop with result
                           for x below 3
-                          and y = (progn (push x result)) then (progn (push (1+ x) result))
+                          and y = (progn (push x result) result)
+                             then (progn (push (1+ x) result) result)
                           finally return result)
                  '(3 2 1 0)))
 

RE: master a6b5985: Avoid duplicated character classes in rx

[Drew Adams]

FWIW, I disagree that it's necessary, or even worthwhile/helpful, to avoid using the return value of `push'.

And I think the return value should be documented.

(This is like not documenting the return value of `progn' or `setq`'. ;-))

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Fri, Dec 6, 2019 at 8:47 PM Drew Adams <drew.adams@oracle.com> wrote:

> FWIW, I disagree that it's necessary, or even worthwhile/helpful,
> to avoid using the return value of `push'.
>
> And I think the return value should be documented.

Well, I would perhaps agree, for CL compatibility' sake. But I think
this is, up to a point, just bikeshedding. As shown, in 99,92% of cases
the return value is not used, and in the 0,08% that it is used, it's
just to avoid

  (progn (push value variable) variable)

or

  (progn (push value variable) t)

And it's a fact that, although push's return value *is* documented in
CL, it has never been so in Emacs, at least going back to cl.el in
Emacs 20 (even back then, there were just vague promises that it was
similar to `setf').

So it'd be nice if it were documented, but it is not, and the few
places that use it can easily be fixed. No big deal, not worth
discussing it IMO.

> (This is like not documenting the return value of `progn' or `setq`'. ;-))

The return value of `progn' is part of its interface and use. As
for `setq', it wouldn't be earthshaking if it wasn't documented.

[-- Attachment #2: Type: text/html, Size: 2052 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Adam Porter]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Fri, Dec 6, 2019 at 8:47 PM Drew Adams <drew.adams@oracle.com> wrote:
>
>> FWIW, I disagree that it's necessary, or even worthwhile/helpful,
>> to avoid using the return value of `push'.
>>
>> And I think the return value should be documented.
>
> Well, I would perhaps agree, for CL compatibility' sake. But I think
> this is, up to a point, just bikeshedding. As shown, in 99,92% of cases
> the return value is not used, and in the 0,08% that it is used, it's
> just to avoid
>
>   (progn (push value variable) variable)
>
> or
>
>   (progn (push value variable) t)
>
> And it's a fact that, although push's return value *is* documented in
> CL, it has never been so in Emacs, at least going back to cl.el in
> Emacs 20 (even back then, there were just vague promises that it was
> similar to `setf').

There are hundreds of thousands, probably millions, of lines of Elisp in
third-party packages and user configurations in the world.  Removing the
return value of `push' would be a breaking change affecting innumerable
users.  It would break the existing, implicit API that has existed since
Emacs 20, released 22 years ago.

And for what?  The sake of not adding "and returns VALUE" to the
docstring and manual?

> So it'd be nice if it were documented, but it is not, and the few
> places that use it can easily be fixed. No big deal, not worth
> discussing it IMO.

By the same logic, it'd be nice if it were documented, and with a few
words added to a few files, it can easily be documented.  No big deal,
not worth discussing it IMO.

>> (This is like not documenting the return value of `progn' or `setq`'. ;-))
>
> The return value of `progn' is part of its interface and use. As
> for `setq', it wouldn't be earthshaking if it wasn't documented.

`push' is a standard Lisp feature and has been for decades.  An
oversight of a few words in the Elisp documentation is not reason to
break over 20 years of Elisp code.

What's going on?  What is the motivation here?

Re: master a6b5985: Avoid duplicated character classes in rx

[Stefan Monnier]

> There are hundreds of thousands, probably millions, of lines of Elisp in
> third-party packages and user configurations in the world.  Removing the
> return value of `push' would be a breaking change affecting innumerable
> users.

That's called a straw man argument: no one suggested "removing the
return value".

>> So it'd be nice if it were documented, but it is not, and the few
>> places that use it can easily be fixed. No big deal, not worth
>> discussing it IMO.
> By the same logic, it'd be nice if it were documented, and with a few
> words added to a few files, it can easily be documented.  No big deal,
> not worth discussing it IMO.

Exactly.  It's a question opinion.  I.e. a bikeshedding subject (which
indeed is more or less synonymous with "not worth discussing" ;-)

When I was maintainer I decided not to document the return value.
Eli is free to make another choice, of course.

But please, let's remember that it's just an opinion.
There's no right or wrong choice.


        Stefan

Re: master a6b5985: Avoid duplicated character classes in rx

[Adam Porter]

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

>> There are hundreds of thousands, probably millions, of lines of Elisp in
>> third-party packages and user configurations in the world.  Removing the
>> return value of `push' would be a breaking change affecting innumerable
>> users.
>
> That's called a straw man argument: no one suggested "removing the
> return value".

In this case, then, not a strawman but a misunderstanding of what was
being proposed.  I thought the return value was being argued against
rather than merely the usage of it.  I'm glad to find that I
misunderstood.  Apologies for the noise.  :)

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

I'm *for* documenting the value of push, but I think discussing it is
pointless.

However, if the value is not documented, relying on it is an error.
So either we document it, or we fix the cases where this happens in
our sources. As for what happens outside our sources, we cannot
control how people uses undocumented features.

[-- Attachment #2: Type: text/html, Size: 697 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 10, 2019 at 4:28 AM Adam Porter <adam@alphapapa.net> wrote:

> There are hundreds of thousands, probably millions, of lines of Elisp in
> third-party packages and user configurations in the world.  Removing the
> return value of `push' would be a breaking change affecting innumerable
> users.

Just for the record, our sources have approx. 1,345,500 non-empty,
non-comment lines of lisp. I found 34 uses of the return value of push
(could be a few more that I didn't find, but not many).

That's one use every ~40,000 lines of code, or approx 0,0025% of them.

If code outside has a similar rating, breaking it wouldn't be really
something that should kept us awake.

But anyway, the point is moot, as non-documenting it just means that the
code is mistaken, and will continue to be so. As Stefan as said, nobody
was talking about modifying push's implementation.

[-- Attachment #2: Type: text/html, Size: 1301 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 6 Dec 2019 19:49:07 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs developers <emacs-devel@gnu.org>
> 
> What I mean is that this isn't, per se, a destabilizing change, but
> there are some subtleties. Eli, do I install it right now, or do you
> prefer to wait after the 27 branch is cut?

Please wait, and thanks.

Re: master a6b5985: Avoid duplicated character classes in rx

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

What does Common Lisp say about the return value of 'push'?
If Common Lisp describes a certain return value for 'push',
people will tend to use it that way, and I think it would be
a waste of time changing all code that ever depends on that return value.
It would be easier to document that return value.


-- 
Dr Richard Stallman
Founder, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Sat, Dec 7, 2019 at 5:48 AM Richard Stallman <rms@gnu.org> wrote:

> What does Common Lisp say about the return value of 'push'?

From the Common Lisp Hyperspec:


*Macro* *PUSH*

*Syntax:*

*push* *item place* => *new-place-value*

*Arguments and Values:*

*item*---an *object* <26_glo_o.htm#object>.

*place*---a *place* <26_glo_p.htm#place>, the *value* <26_glo_v.htm#value> of
which may be any *object* <26_glo_o.htm#object>.

*new-place-value*---a *list* <26_glo_l.htm#list> (the new *value*
<26_glo_v.htm#value> of *place*).

*Description:*

*push* <#push> prepends *item* to the *list* <26_glo_l.htm#list> that is
stored in *place*, stores the resulting *list* <26_glo_l.htm#list> in
*place*, and returns the *list* <26_glo_l.htm#list>.


> If Common Lisp describes a certain return value for 'push',
> people will tend to use it that way,

Yes, though as I said, it is very rarely used, at least on our sources. 34
uses
out of ~4,100.

There are also a lot like

(and test-1
     test-2
     ...
     (push item place))

but don't really depend on the return value of `push', they're just using
`and'
as a conditional.

> and I think it would be
> a waste of time changing all code that ever depends on that return value.

At least for our sources, that time's already been wasted, whether we apply
my
patch or not.

> It would be easier to document that return value.

I tend to agree, because I don't like gratuitously discarding CL
compatibility.

[-- Attachment #2: Type: text/html, Size: 4200 bytes --]

RE: master a6b5985: Avoid duplicated character classes in rx

[Drew Adams]

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

(BTW - Even a purely functional definition of `push' would return the new stack/list value. And users would of course make use of that return value.)

___

 

Besides the Hyperspec, Common Lisp the Language says this (and it's the case also for the first, 1984 edition, not just CLTL2):

[Macro]
push item place

The form place should be the name of a generalized variable containing a list; item may refer to any Lisp object. The item is consed onto the front of the list, and the augmented list is stored back into place and returned. The form place may be any form acceptable as a generalized variable to setf. If the list held in place is viewed as a push-down stack, then push pushes an element onto the top of the stack. For example:

(setq x '(a (b c) d)) 

(push 5 (cadr x)) => (5 b c)  and now x => (a (5 b c) d)

The effect of (push item place) is roughly equivalent to

(setf place (cons item place))

except that the latter would evaluate any subforms of place twice, while push takes care to evaluate them only once. Moreover, for certain place forms push may be significantly more efficient than the setf version.

 

On Sat, Dec 7, 2019 at 5:48 AM Richard Stallman <HYPERLINK "mailto:rms@gnu.org"rms@gnu.org> wrote:

> What does Common Lisp say about the return value of 'push'?

From the Common Lisp Hyperspec:

 

Macro PUSH

Syntax:

push item place => new-place-value

Arguments and Values:

item---an object.

place---a place, the value of which may be any object.

new-place-value---a list (the new value of place).

Description:

HYPERLINK \l "push prepends item to the list that is stored in place, stores the resulting list in place, and returns the list.

> If Common Lisp describes a certain return value for 'push',
> people will tend to use it that way,

Yes, though as I said, it is very rarely used, at least on our sources. 34 uses out of ~4,100.

There are also a lot like
(and test-1 test-2 ... (push item place))

but don't really depend on the return value of `push', they're just using `and' as a conditional.

 

> and I think it would be a waste of time changing all code that ever depends on that return value.

 

At least for our sources, that time's already been wasted, whether we apply my patch or not.

 

Emacs Lisp is not just for "our sources". It's for Emacs users. What we show as the doc of a function matters for users, not just for Emacs internal developers.

 

> It would be easier to document that return value.

 

Not just easier. Better, including for users.

I tend to agree, because I don't like gratuitously discarding CL compatibility.

[-- Attachment #2: Type: text/html, Size: 11933 bytes --]

Re: master a6b5985: Avoid duplicated character classes in rx

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > It would be easier to document that return value.

  > I tend to agree, because I don't like gratuitously discarding CL
  > compatibility.

I think we have highlighted two different aspects of that same point.

-- 
Dr Richard Stallman
Founder, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

RE: master a6b5985: Avoid duplicated character classes in rx

[Drew Adams]

> > I'd rather fix the code not to rely on the return value.
> 
> I'm sure a lot more code relies on the return value of 'push'.
> It's also well-defined in Common Lisp. Why not just document it?

+1.

My guess is that this doc lack might have just
been an oversight.

Prior to Emacs 21 (or possibly 22), `push' was
defined only in `cl.el' (with no `cl-' prefix).

Its doc string said this:

  push is a Lisp macro in `cl.el'.
  (push X PLACE)

  (push X PLACE): insert X at the head of the list stored in PLACE.
  Analogous to (setf PLACE (cons X PLACE)), though more careful about
  evaluating each argument only once and in the right order.  PLACE may
  be a symbol, or any generalized variable allowed by `setf'.

IOW, even though that version of `push' was
supposed to be an Emacs emulation of Common
Lisp `push', there's no mention of the return
value.  There should have been.

`push' was moved outside of `cl.el' in Emacs 22
(or 21 possibly).  Its doc string was not fixed
to specify the return value.

`cl-pushnew' has the same doc problem: no spec
of the return value.

We should specify the return value for each of
these.  It is as important to `push' as it is
to `pop' (whose doc does specify the return
value).

Common Lisp specifies the return value:

http://clhs.lisp.se/Body/m_push.htm

Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero]

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

On Tue, Dec 3, 2019 at 4:26 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> I'd rather fix the code not to rely on the return value.

Agreed, but I wasn't talking specifically about this case. There are others
in the code,
I think. If we don't want to document the return value of push, we should
fix them all.

>     +                    (and class
>     +                         (progn (cl-pushnew class classes) t))))

Thinking about converting it to cl-pushnew is what made me look closer at
this code in
the first place ;-)

[-- Attachment #2: Type: text/html, Size: 937 bytes --]