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

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

[ I wrote this yesterday already, but somehow it got eaten by Murphy or
  something.  ]

>   cl-lib defstruct introspection

Please always use the ChangeLog entry as commit message.

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

I'm curious: when/where did you bump against a need for that?

> +@defun cl-struct-set-slot-value struct-type slot-name inst value

We don't need this, since we can always use setf instead.

> -(defun cl--const-expr-val (x)
> +(defun cl--const-expr-val (x &optional environment default)

`environment' is always macroexpand-all-environment, and `default' is
never used, so you can revert this part of the change.

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

Don't go past the 80th column, please.  Undoing the cl--const-expr-val
signature change will fix this problem in most of your patch.

> +(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)))
> -  form)
> +  (if (not (or (not (cl--compiling-file))

Unless absolutely needed, we should drop this (cl--compiling-file) test,
because cl--compiling-file is an ugly hack.

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

Please add `side-effect-free' to defun-declarations-alist, so we can
move these into `declare's, which is cleaner (it associates them with
the function itself rather than with the shared symbol).

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

Please use (declare (gv-setter ...)) or (declare (gv-expand ...)).
Also, we should define it better such that (incf (cl-struct-slot-value ..))
only computes the offset and tests the type once.

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

Please use (declare (compiler-macro ..)).

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

How important is this optimization?  I mean, if struct-type and
slot-name are constants, then presumably, the code could have used the
accessor function instead, no?
I guess this goes back to the earlier question about when/where the use
for this functionality came up.


        Stefan