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

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

Oh, well, looks like the original answer wasn't completely lost
after all.  It was just stuck in some mail queue!
Make sure you disregard it, tho.  I think we've hashed this enough already.


        Stefan

>>>>> "Stefan" == Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> 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?

>> +@code{setf} place.  @code{cl-struct-slot-value} uses
>> +@code{cl-struct-slot-offset} 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