> 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).
0001-Add-gv-define-expander-for-plist-get.patch