Re: [Emacs-diffs] emacs-26 59e8533 1/3: Add save-match-data to abbreviate-file-name (Bug#32201)

Re: [Emacs-diffs] emacs-26 59e8533 1/3: Add save-match-data to abbreviate-file-name (Bug#32201)

[Stefan Monnier]

> @@ -1929,7 +1929,7 @@ started Emacs, set `abbreviated-home-dir' to nil so it will be recalculated)."
>  			 (save-match-data
>  			   (string-match "^[a-zA-`]:/$" filename))))
>                 (equal (get 'abbreviated-home-dir 'home)
> -                      (expand-file-name "~")))
> +                      (save-match-data (expand-file-name "~"))))
>  	  (setq filename
>  		(concat "~"
>  			(match-string 1 filename)

Wouldn't it better to read (match-string 1 filename) earlier?
`save-match-data` is a costly operation compared to (match-string
1 filename), so it doesn't make much sense to use it everywhere
between the match and the final (match-string 1 filename).

As a general rule, code of the form

    ...regexp match...
    ...then save-match-data...
    ...then use match-data...

is better avoided: save-match-data is only really useful in those cases
where the regexp-match and the use-match-data are completely elsewhere
(and for some reason we have decided that it's better to make this
function match-data-preserving rather than just change the callers,
which is usually easy to do and more efficient).

See patch below for instance (which even avoids the allocation when
it's not needed).

files.el is riddled with save-match-data in a way that doesn't make
sense w.r.t the above code: either we save-match-data around the body of
every file-operation function, or we save match around the calls to
file-operations, but doing both is a bad idea (especially since
in 99% of the cases we can get away with doing neither).


        Stefan


diff --git a/lisp/files.el b/lisp/files.el
index b8f6c46146..1db85295a1 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1895,7 +1896,7 @@ abbreviate-file-name
 if you want to permanently change your home directory after having
 started Emacs, set `abbreviated-home-dir' to nil so it will be recalculated)."
   ;; Get rid of the prefixes added by the automounter.
-  (save-match-data
+  (save-match-data                      ;FIXME: Why?
     (if (and automount-dir-prefix
 	     (string-match automount-dir-prefix filename)
 	     (file-exists-p (file-name-directory
@@ -1946,21 +1947,21 @@ abbreviate-file-name
       ;; is likely temporary (eg for testing).
       ;; FIXME Is it even worth caching abbreviated-home-dir?
       ;; Ref: https://debbugs.gnu.org/19657#20
-      (if (and (string-match abbreviated-home-dir filename)
-	       ;; If the home dir is just /, don't change it.
-	       (not (and (= (match-end 0) 1)
-			 (= (aref filename 0) ?/)))
-	       ;; MS-DOS root directories can come with a drive letter;
-	       ;; Novell Netware allows drive letters beyond `Z:'.
-	       (not (and (memq system-type '(ms-dos windows-nt cygwin))
-			 (save-match-data
-			   (string-match "^[a-zA-`]:/$" filename))))
-               (equal (get 'abbreviated-home-dir 'home)
-                      (save-match-data (expand-file-name "~"))))
-	  (setq filename
-		(concat "~"
-			(match-string 1 filename)
-			(substring filename (match-end 0)))))
+      (let (mb1)
+        (if (and (string-match abbreviated-home-dir filename)
+                 (setq mb1 (match-beginning 1))
+	         ;; If the home dir is just /, don't change it.
+	         (not (and (= (match-end 0) 1)
+			   (= (aref filename 0) ?/)))
+	         ;; MS-DOS root directories can come with a drive letter;
+	         ;; Novell Netware allows drive letters beyond `Z:'.
+	         (not (and (memq system-type '(ms-dos windows-nt cygwin))
+			   (string-match "\\`[a-zA-`]:/\\'" filename)))
+                 (equal (get 'abbreviated-home-dir 'home)
+                        (expand-file-name "~")))
+	    (setq filename
+		  (concat "~"
+			  (substring filename mb1)))))
       filename)))
 
 (defun find-buffer-visiting (filename &optional predicate)
@@ -7148,7 +7149,7 @@ file-name-non-special
     (if (symbolp (car file-arg-indices))
 	(setq method (pop file-arg-indices)))
     ;; Strip off the /: from the file names that have it.
-    (save-match-data
+    (save-match-data                    ;FIXME: Why?
       (while (consp file-arg-indices)
 	(let ((pair (nthcdr (car file-arg-indices) arguments)))
 	  (when (car pair)

Re: [Emacs-diffs] emacs-26 59e8533 1/3: Add save-match-data to abbreviate-file-name (Bug#32201)

[Noam Postavsky]

On Tue, 16 Oct 2018 at 21:50, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > @@ -1929,7 +1929,7 @@ started Emacs, set `abbreviated-home-dir' to nil so it will be recalculated)."
> >                        (save-match-data
> >                          (string-match "^[a-zA-`]:/$" filename))))
> >                 (equal (get 'abbreviated-home-dir 'home)
> > -                      (expand-file-name "~")))
> > +                      (save-match-data (expand-file-name "~"))))
> >         (setq filename
> >               (concat "~"
> >                       (match-string 1 filename)
>
> Wouldn't it better to read (match-string 1 filename) earlier?
> `save-match-data` is a costly operation compared to (match-string
> 1 filename), so it doesn't make much sense to use it everywhere
> between the match and the final (match-string 1 filename).

Yeah, I think I kind of added it mindlessly, following the
save-match-data in the previous clause.