Re: [Emacs-diffs] trunk r116995: cl-lib defstruct introspection

[Stefan Monnier]

Thanks, Daniel.  A few comments about this new facility.

>   cl-lib defstruct introspection

This is rather short: The commit message should be a copy of the
ChangeLog text.

> +The @code{cl-defstruct} package also provides a few structure
> +introspection functions.

Curious: when/where did you bump into this need?

> address@hidden place.  @code{cl-struct-slot-value} uses
> address@hidden internally and can signal the same
> +errors.

Don't say what it does internally: just mention that it signals the same
errors as cl-struct-slot-offset.  Same for cl-struct-set-slot-value
(even more so there, since you copy&pasted the text without updating 
cl-struct-slot-value to cl-struct-set-slot-value).

> -                                      `'(nil ,(cl--const-expr-val def))
> +                                      `'(nil ,(cl--const-expr-val
> +                                                  def 
> macroexpand-all-environment))

Please stay within 80 columns.

> +(defmacro cl-the (type form)
> +  "Return FORM.  If type-checking is enabled, assert that it is of TYPE."
>    (declare (indent 1) (debug (cl-type-spec form)))
> +  (if (not (or (not (cl--compiling-file))

Do we really need this cl--compiling-file here?  If it's not absolutely
indispensible, I'd rather avoid using it, since its implementation is
a hideous unreliable hack.

this pu> +(defun cl-struct-sequence-type (struct-type)
> +  "Return the sequence used to build STRUCT-TYPE.
> +STRUCT-TYPE is a symbol naming a struct type.  Return 'vector or
> +'list, or nil if STRUCT-TYPE is not a struct type. "
> +  (car (get struct-type 'cl-struct-type)))
> +(put 'cl-struct-sequence-type 'side-effect-free t)

Could you add a `side-effect-free' thingy to defun-declarations-alist and move
these `put' into a declare?

> +(defsetf cl-struct-slot-value cl-struct-set-slot-value)

Please don't use cl.el facilities here.  Use (declare (gv-setter ...))
instead.  Also, the setter should be improved such that
(incf (cl-struct-slot-value ...)) only computes the offset once.

> +(cl-define-compiler-macro cl-struct-slot-value
> +(cl-define-compiler-macro cl-struct-set-slot-value

These should use the (declare (compiler-macro ...)) facility.
This said, I wonder why they're needed (gets us back to my earlier
question about when you bumped into a need for these facilities).

> +    (&whole orig struct-type slot-name inst)
> +  (or (let* ((macenv macroexpand-all-environment)
> +             (struct-type (cl--const-expr-val struct-type macenv))
> +             (slot-name (cl--const-expr-val slot-name macenv)))
> +        (and struct-type (symbolp struct-type)
> +             slot-name (symbolp slot-name)
> +             (assq slot-name (cl-struct-slot-info struct-type))
> +             (let ((idx (cl-struct-slot-offset struct-type slot-name)))
> +               (cl-ecase (cl-struct-sequence-type struct-type)
> +                 (vector `(aref (cl-the ,struct-type ,inst) ,idx))
> +                 (list `(nth ,idx (cl-the ,struct-type ,inst)))))))
> +      orig))
[...]
> +    (&whole orig struct-type slot-name inst value)
> +  (or (let* ((macenv macroexpand-all-environment)
> +             (struct-type (cl--const-expr-val struct-type macenv))
> +             (slot-name (cl--const-expr-val slot-name macenv)))
> +        (and struct-type (symbolp struct-type)
> +             slot-name (symbolp slot-name)
> +             (assq slot-name (cl-struct-slot-info struct-type))
> +             (let ((idx (cl-struct-slot-offset struct-type slot-name)))
> +               (cl-ecase (cl-struct-sequence-type struct-type)
> +                 (vector `(setf (aref (cl-the ,struct-type ,inst) ,idx)
> +                                ,value))
> +                 (list `(setf (nth ,idx (cl-the ,struct-type ,inst))
> +                              ,value))))))
> +      orig))

Try to share code between these two.


        Stefan