Re: [PATCH] Add gv-define-expander for plist-get

[Naoya Yamashita <conao3@gmail.com>]

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

> Sorry for not answering earlier.

I thanks you for your great review!

> The patch looks good (with special thanks for the tests), except that in
> the code above, you shouldn't have those (repeated) `(plist-member
> ,plist ,key)` since they not only waste time but they will also evaluate
> `plist` multiple times which could change the semantics in case `plist`
> has side effects.
> I think these should refer to `p`, right?

I think good too.

> Furthermore, I think you need to use ',key rather than just ,key in case
> the key is a cons or a plain symbol, so maybe you want to add a test
> that uses symbols rather than keywords:
>
>     ;; Other function (cl-rotatef) usage for plist-get.
>     (should (equal (let ((target '(a "a" b "b" c "c")))
>                      (cl-rotatef (plist-get target 'b) (plist-get target
'c))
>                      target)
>                    '(a "a" b "c" c "b")))

I cannot understand this point.  Your additional test is passed
by current code, the additional quote is unneeded I think.

If we add quote for `key`, the below code will be valid.

    (let ((target '(a "a" b "b" c "c")))
      (setf (plist-get target b) "modify")
      target)
    ;;=> (a "a" b "modify" c "c")

but this code will be invalid.

    (let ((target '(a "a" b "b" c "c")))
      (setf (plist-get target 'b) "modify")
      target)
    ;;=> ('b "modify" a "a" b "b" c "c")

I think the latter should be valid and the former should be
invalid.  This is an analogy from normal usage of plist-get.

    (let ((target '(a "a" b "b" c "c")))
      (plist-get target b))
    ;;=> Debugger entered--Lisp error: (void-variable b)
    ;;     (plist-get target b)
    ;;     (let ((target '(a "a" b "b" c "c"))) (plist-get target b))
    ;;     (progn (let ((target '(a "a" b "b" c "c"))) (plist-get target
b)))
    ;;     eval((progn (let ((target '(a "a" b "b" c "c"))) (plist-get
target b))) t)

    (let ((target '(a "a" b "b" c "c")))
      (plist-get target 'b))
    ;;=> "b"

> Also, I'd add a `cdr` to the computation of `p` so that you don't need
> to do that `cdr` in both the getter and the setter.

Thanks.  I fix code based your advice.

Finally, here is diff from last time commit.
(Attached diff is squached diff.
 You can use this diff on last time commit if you prefer.)

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index d52097575d..568bcf244b 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -421,13 +421,13 @@ plist-get
   (lambda (do plist prop)
     (macroexp-let2 macroexp-copyable-p key prop
       (gv-letplace (getter setter) plist
-        (macroexp-let2 nil p `(plist-member ,getter ,key)
+        (macroexp-let2 nil p `(cdr (plist-member ,getter ',key))
           (funcall do
                    `(cadr ,p)
                    (lambda (val)
-                     `(if (plist-member ,plist ,key) (setcar (cdr
(plist-member ,plist ,key)) ,val)
-                        ,(funcall setter `(cons ,key (cons ,val
,getter)))))))))))
+                     `(if ,p
+                          (setcar ,p ,val)
+                        ,(funcall setter `(cons ',key (cons ,val
,getter)))))))))))

 ;;; Some occasionally handy extensions.

diff --git a/test/lisp/emacs-lisp/gv-tests.el
b/test/lisp/emacs-lisp/gv-tests.el
index a9ceb7f730..10e3b531f3 100644
--- a/test/lisp/emacs-lisp/gv-tests.el
+++ b/test/lisp/emacs-lisp/gv-tests.el
@@ -182,7 +182,19 @@ gv-plist-get
   (should (equal (let ((target '(:a "a" :b "b" :c "c")))
                    (cl-rotatef (plist-get target :b) (plist-get target :d))
                    target)
-                 '(:d "b" :a "a" :b nil :c "c"))))
+                 '(:d "b" :a "a" :b nil :c "c")))
+
+  ;; Simple setf usage for plist-get. (symbol plist)
+  (should (equal (let ((target '(a "a" b "b" c "c")))
+                   (setf (plist-get target 'b) "modify")
+                   target)
+                 '(a "a" b "modify" c "c")))
+
+  ;; Other function (cl-rotatef) usage for plist-get. (symbol plist)
+  (should (equal (let ((target '(a "a" b "b" c "c")))
+                   (cl-rotatef (plist-get target 'b) (plist-get target 'c))
+                   target)
+                 '(a "a" b "c" c "b"))))

 ;; `ert-deftest' messes up macroexpansion when the test file itself is
 ;; compiled (see Bug #24402).


2020年9月6日(日) 3:03 Stefan Monnier <monnier@iro.umontreal.ca>:

> Hi,
>
> Sorry for not answering earlier.
>
> > I find `gv` has `gv-define-expander` for `alist-get`, but
> > there're no definition for `plist-get`
>
> Indeed.
>
> > +(gv-define-expander plist-get
> > +  (lambda (do plist prop)
> > +    (macroexp-let2 macroexp-copyable-p key prop
> > +      (gv-letplace (getter setter) plist
> > +        (macroexp-let2 nil p `(plist-member ,getter ,key)
> > +          (funcall do
> > +                   `(cadr ,p)
> > +                   (lambda (val)
> > +                     `(if (plist-member ,plist ,key) (setcar (cdr
> (plist-member ,plist ,key)) ,val)
> > +                        ,(funcall setter `(cons ,key (cons ,val
> ,getter)))))))))))
>
> The patch looks good (with special thanks for the tests), except that in
> the code above, you shouldn't have those (repeated) `(plist-member
> ,plist ,key)` since they not only waste time but they will also evaluate
> `plist` multiple times which could change the semantics in case `plist`
> has side effects.
> I think these should refer to `p`, right?
>
> Furthermore, I think you need to use ',key rather than just ,key in case
> the key is a cons or a plain symbol, so maybe you want to add a test
> that uses symbols rather than keywords:
>
>     ;; Other function (cl-rotatef) usage for plist-get.
>     (should (equal (let ((target '(a "a" b "b" c "c")))
>                      (cl-rotatef (plist-get target 'b) (plist-get target
> 'c))
>                      target)
>                    '(a "a" b "c" c "b")))
>
> Also, I'd add a `cdr` to the computation of `p` so that you don't need
> to do that `cdr` in both the getter and the setter.
>
>
> -- Stefan
>
>

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

[-- Attachment #2: 0001-Add-gv-define-expander-for-plist-get.patch --]
[-- Type: text/x-patch, Size: 3547 bytes --]

From d83ee0224c3d239d2713103c13c71a9e048f239a Mon Sep 17 00:00:00 2001
From: Naoya Yamashita <conao3@gmail.com>
Date: Wed, 9 Sep 2020 09:52:39 +0900
Subject: [PATCH] Add gv-define-expander for plist-get

It is necessary to make plist-get as a generalized variable, and this
definition allows user to use setf and other useful functions on
plist-get.

* lisp/emacs-lisp/gv.el: Add gv-define-expander for plist-get

* lisp/emacs-lisp/gv-tests.el: Add new tests for plist-get
---
 lisp/emacs-lisp/gv.el            | 11 +++++++++
 test/lisp/emacs-lisp/gv-tests.el | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index 78d86b9fc3..568bcf244b 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -417,6 +417,17 @@ alist-get
                                               `(delq ,p ,getter))))))
                             ,v))))))))))
 
+(gv-define-expander plist-get
+  (lambda (do plist prop)
+    (macroexp-let2 macroexp-copyable-p key prop
+      (gv-letplace (getter setter) plist
+        (macroexp-let2 nil p `(cdr (plist-member ,getter ',key))
+          (funcall do
+                   `(cadr ,p)
+                   (lambda (val)
+                     `(if ,p
+                          (setcar ,p ,val)
+                        ,(funcall setter `(cons ',key (cons ,val ,getter)))))))))))
 
 ;;; Some occasionally handy extensions.
 
diff --git a/test/lisp/emacs-lisp/gv-tests.el b/test/lisp/emacs-lisp/gv-tests.el
index 7a8402be07..10e3b531f3 100644
--- a/test/lisp/emacs-lisp/gv-tests.el
+++ b/test/lisp/emacs-lisp/gv-tests.el
@@ -156,6 +156,46 @@ gv-setter-edebug
       (eval-buffer)))
   (should (equal (get 'gv-setter-edebug 'gv-setter-edebug-prop) '(123))))
 
+(ert-deftest gv-plist-get ()
+  (require 'cl-lib)
+
+  ;; Simple setf usage for plist-get.
+  (should (equal (let ((target '(:a "a" :b "b" :c "c")))
+                   (setf (plist-get target :b) "modify")
+                   target)
+                 '(:a "a" :b "modify" :c "c")))
+
+  ;; Other function (cl-rotatef) usage for plist-get.
+  (should (equal (let ((target '(:a "a" :b "b" :c "c")))
+                   (cl-rotatef (plist-get target :b) (plist-get target :c))
+                   target)
+                 '(:a "a" :b "c" :c "b")))
+
+  ;; Add new key value pair at top of list if setf for missing key.
+  (should (equal (let ((target '(:a "a" :b "b" :c "c")))
+                   (setf (plist-get target :d) "modify")
+                   target)
+                 '(:d "modify" :a "a" :b "b" :c "c")))
+
+  ;; Rotate with missing value.
+  ;; The value corresponding to the missing key is assumed to be nil.
+  (should (equal (let ((target '(:a "a" :b "b" :c "c")))
+                   (cl-rotatef (plist-get target :b) (plist-get target :d))
+                   target)
+                 '(:d "b" :a "a" :b nil :c "c")))
+
+  ;; Simple setf usage for plist-get. (symbol plist)
+  (should (equal (let ((target '(a "a" b "b" c "c")))
+                   (setf (plist-get target 'b) "modify")
+                   target)
+                 '(a "a" b "modify" c "c")))
+
+  ;; Other function (cl-rotatef) usage for plist-get. (symbol plist)
+  (should (equal (let ((target '(a "a" b "b" c "c")))
+                   (cl-rotatef (plist-get target 'b) (plist-get target 'c))
+                   target)
+                 '(a "a" b "c" c "b"))))
+
 ;; `ert-deftest' messes up macroexpansion when the test file itself is
 ;; compiled (see Bug #24402).
 
-- 
2.28.0