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

[Daniel Colascione]

On 04/21/2014 08:38 AM, Stefan Monnier wrote:
> [ 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.

That's new. Using the whole ChaneLog message has been a recommendation,
but never a requirement. Now there's one more step on the commit path,
and a useless one at that: the changelog entry is available in the
change itself and in the message to the mailing list.

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

I have a few private macros that lexically bind structure slots, and
this information is also needed for some interface-generation work I'm
thinking of doing.

>> address@hidden cl-struct-set-slot-value struct-type slot-name inst value
> 
> We don't need this, since we can always use setf instead.

So? We have both (setf (aref ...) ...) and (aset ...).

>> -(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.

Sure.

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

That test was there in cl-check-type. The test doesn't make sense to me
either. We should drop it in both places if we drop it in cl-the.

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

Sure.

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

I've already changed it to gv-define-simple-setter. I don't think the
incf optimization you mention matters.

>> +(cl-define-compiler-macro cl-struct-slot-value
> 
> Please use (declare (compiler-macro ..)).

Why? In both cases, the compiler macro is written out-of-line and in
both cases, we just stick the compiler macro on the symbol's plist.

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

Unless we're using this functionality in generated code where, while the
slot is constant, it's more convenient to use that slot's name than to
try to determine the accessor name.

signature.asc
Description: OpenPGP digital signature