GNU bug report logs - #77058
[PATCH] Add cl-with-accessors

Previous Next

Full log

Message #14 received at 77058 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: okamsn <at> protonmail.com, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77058 <at> debbugs.gnu.org
Subject: Re: bug#77058: [PATCH] Add cl-with-accessors
Date: Sat, 29 Mar 2025 14:29:06 +0300

Ping! Okamsn, would you like to submit an updated patch?

> Cc: 77058 <at> debbugs.gnu.org
> Date: Tue, 18 Mar 2025 12:19:56 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> > 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
> 
> 
> 
> 
> 

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.