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

[Stefan Monnier]

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