unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Daniel Colascione <dancol@dancol.org>
To: Stefan Monnier <monnier@IRO.UMontreal.CA>
Cc: emacs-devel@gnu.org
Subject: Re: [Emacs-diffs] trunk r116995: cl-lib defstruct introspection
Date: Mon, 21 Apr 2014 10:40:50 -0700	[thread overview]
Message-ID: <53555822.3080007@dancol.org> (raw)
In-Reply-To: <jwvy4yypvd6.fsf-monnier+emacsdiffs@gnu.org>

[-- Attachment #1: Type: text/plain, Size: 4173 bytes --]

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.

>> +@defun 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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

  reply	other threads:[~2014-04-21 17:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <E1Wbhqn-0001CJ-4X@vcs.savannah.gnu.org>
2014-04-20 12:49 ` [Emacs-diffs] trunk r116995: cl-lib defstruct introspection Stefan Monnier
2014-04-23 12:56   ` Stefan Monnier
2014-04-21 15:38 ` Stefan Monnier
2014-04-21 17:40   ` Daniel Colascione [this message]
2014-04-21 22:03     ` Stefan Monnier
2014-04-21 22:26       ` Daniel Colascione
2014-04-22  2:03         ` Stefan Monnier
2014-04-22  2:07           ` Daniel Colascione
2014-04-22  3:28             ` Stefan Monnier
2014-04-23  3:18       ` Declaim and proclaim (Was: Re: [Emacs-diffs] trunk r116995: cl-lib defstruct introspection) Daniel Colascione
2014-04-23 13:14         ` Declaim and proclaim Stefan Monnier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53555822.3080007@dancol.org \
    --to=dancol@dancol.org \
    --cc=emacs-devel@gnu.org \
    --cc=monnier@IRO.UMontreal.CA \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).