GNU bug report logs - #70524
[PATCH] Fix `map-elt` with `setf` for subplaces

Previous Next

Full log

Message #8 received at 70524 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text
 editors" <bug-gnu-emacs <at> gnu.org>
Cc: Okamsn <okamsn <at> protonmail.com>, 70524 <at> debbugs.gnu.org
Subject: Re: bug#70524: [PATCH] Fix `map-elt` with `setf` for subplaces
Date: Wed, 24 Apr 2024 08:06:11 +0200

Hello Okamsn,

let's CC Stefan.  Nice to see you are working on this stuff.

> Hello,
>
> Currently, the use
>
>      (let ((arr (vector 0 1 2 3 4 5 6)))
>        (setf (map-elt (cl-subseq arr 3) 0)
>              27)
>        arr)
>
> expands to [...]

But... I must admit I'm not really convinced that this has to be
changed.

First, it is not crystal clear what the semantics should be in this
case, because `cl-subseq' as a function creates a new sequence.  But
it's also a gv so it's debatable, ok.

But second - doesn't your patch lead to very inefficient code in this
example, where nearly all elements of the original sequence get replaced
by themselves in a loop (through the setter of `cl-subseq')?

Maybe there are other examples.  But cases where the first argument
given to `map-elt' returns a part of the original structure (like e.g. a
`car' call) work (and there the semantics is also clearer).

So I wonder if this change is really an improvement.

But if we install it,


---
 lisp/emacs-lisp/map.el            | 6 +++++-
 test/lisp/emacs-lisp/map-tests.el | 5 +++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index d3d71a36ee4..facfdd8de7b 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -167,7 +167,11 @@ map-elt
                        `(condition-case nil
                             ;; Silence warnings about the hidden 4th arg.
                             (with-no-warnings
-                              (map-put! ,mgetter ,key ,v ,testfn))
+                              ,(macroexp-let2 nil m mgetter
+                                 `(progn
+                                    (map-put! ,m ,key ,v ,testfn)
+                                    ,(funcall msetter m)
+                                    ,v)))
                           (map-not-inplace

I guess you should move the `with-no-warnings' wrapper along with the
comment to the inside, around the `map-put!' it is intended for.

Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.