Re: master a6b5985: Avoid duplicated character classes in rx

[Juanma Barranquero <lekktu@gmail.com>]

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