[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Stefan Monnier
Subject: Re: [PATCH] Add gv-define-expander for plist-get
Date: Sat, 05 Sep 2020 14:03:36 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)


Sorry for not answering earlier.

> I find `gv` has `gv-define-expander` for `alist-get`, but
> there're no definition for `plist-get`


> +(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))
                   '(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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]