Subject: Re: bug#77058: [PATCH] Add cl-with-accessors

From: Stefan Monnier <[email-masked]>
Date: Tue, 18 Mar 2025 12:19:56 -0400

> The attached patch adds the macro `cl-with-accessors'.  This macro uses 
> `cl-symbol-macrolet' to treat a symbol like a use of an accessor 
> function, such as those made by `cl-defstruct' and `cl-defclass'. This 
> can be helpful when using repeated calls to read and write with the 
> functions.

Thanks.  See comments below.

> Also, in the test `cl-lib-struct-accessors' in 
> test/lisp/emacs-lisp/cl-lib-tests.el, it checks for the keyword 
> `:readonly', but the documented keyword is `:read-only'.  Is this a 
> typo, or was this intentional?

I'd go with "typo".

> --- a/doc/misc/cl.texi
> +++ b/doc/misc/cl.texi
> @@ -4225,6 +4225,19 @@ Structures
>  
>  Other slot options are currently ignored.
>  
> +The macro @code{cl-with-accessors} can simplify code that repeatedly
> +accesses slots.
> +
> +@example
> +(defun add-one-year (person)
> +  (cl-with-accessors ((age person-age))
> +      person
> +    (if (null age)
> +        (error "Age is missing")
> +      (cl-incf age)))
> +  person)
> +@end example

It'd be worthwhile to mention that it also works with accessors of other
objects than structs, e.g. `car`, or `process-filter`.

Of course, any EIEIO user would rewrite the above to:

    (defun add-one-year (person)
      (with-slots (age) person
        (if (null age)
            (error "Age is missing")
          (cl-incf age)))
      person)

so maybe it's worth mentioning `with-slots` (which arguably should be
moved to `cl-lib`).

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2496,6 +2496,13 @@ This function is like 'type-of' except that it sometimes returns
>  a more precise type.  For example, for nil and t it returns 'null'
>  and 'boolean' respectively, instead of just 'symbol'.
>  
> +** New macro 'cl-with-accessors'
> +This macro is similar to 'map-let', but works with structures and their
> +accessors functions.  It is useful when slots' accessor functions are
> +use repeatedly, such as reading from a slot and then writing to that
> +slot.  Symbol macros are created for the accessor functions using
> +'cl-symbol-macrolet', so that they can be used with 'setq' and 'setf'.

I think this should be moved to a CL-Lib subsection in the
"package-specific changes" section.

> +;;;###autoload
> +(defmacro cl-with-accessors (bindings instance &rest body)
> +  "Generate code using symbols as accessing expressions for INSTANCE inside BODY.
> +
> +This macro helps when writing code that makes repeated use of the
> +accessor functions of a structure or object instance, such as those
> +created by `cl-defstruct' and `defclass'.
> +
> +Inside BODY, SYMBOL-NAME is treated as the function call (ACCESSOR-NAME
> +INSTANCE) using `cl-symbol-macrolet'.  SYMBOL-NAME can be used with
> +`setf' and `setq' as a generalized variable.

I'd use `VAR` or `SYMBOL` rather than `SYMBOL-NAME`.
Probably similarly use `ACCESSOR` rather than `ACCESSOR-NAME`.

> +\(fn ((SYMBOL-NAME ACCESSOR-NAME) ...) INSTANCE &REST BODY)"

Should be lower case `&rest` on this last line.

> +  (declare (debug [(&rest (symbolp place)) form body])
> +           (indent 2))

I think `place` is wrong here.  `(person-age FOO)` is a "place" but
`person-age` is not (or, well, it is but not the right one: when taken
as a place it refers to the variable, not the accessor).
I think it should just be `symbolp`.

> +  (cond ((null body)
> +         (macroexp-warn-and-return "No body given" nil 'empty-body))

Please use the same format as other similar warnings we have, such as:

    (format-message "`unless' with empty body")

- Stefan