Re: [PATCH] pcase.el: Add type pattern

[Adam Porter <adam@alphapapa.net>]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> My understanding was that adding the (require 'cl-lib) form was
>> suggested if the definition was left in pcase.el, and if the definition
>> were added to cl-macs.el instead, the (require 'cl-lib) form would not
>> be needed.
>
> Oh, I missed that, then sorry for the noise.

No problem.  Thanks.

>> 1.  Run emacs -Q.
>> 2.  Evaluate this form in *scratch*:
>>
>>   (pcase-defmacro type (type)
>>     "Pcase pattern that matches objects of TYPE.
>>   TYPE is a symbol or list as accepted by `cl-typep', which see."
>>     `(pred (pcase--flip cl-typep ',type)))
>>
>> 3.  Evaluate this form in *scratch*:
>>
>>   (macroexpand-all '(pcase 10 ((type (integer 0 10)) t)))
>>
>> Result:
>>
>>   (if (and (integerp 10) (>= 10 (quote 0)) (<= 10 (quote 10))) (progn t)
>>   nil)
>
> No, I don't see that here.  I'm using current master here.

Hm, I don't know how to explain that.  I guess I'll have to rebuild
master and check again.

>> However, I have attached another version of the patch which adds an
>> autoload for the (pcase-defmacro type) form, which seems proper since
>> the (pcase-defmacro cl-struct) form in the same file also has one.
>
> What effect does this have?  Since it's no defmacro from, the form is
> just copied literally to the generated loaddefs file, right?  Wouldn't
> that make a an explicit `require' in the definition necessary?

The (pcase-defmacro type) form expands to:

  (progn
    (defun type--pcase-macroexpander
        (type)
      ;; [long docstring omitted here]
      `(pred
        (pcase--flip cl-typep ',type)))
    (define-symbol-prop 'type--pcase-macroexpander 'edebug-form-spec 'nil)
    (define-symbol-prop 'type 'pcase-macroexpander #'type--pcase-macroexpander))

My understanding is that the autoload cookie causes the entire
cl-macs.el file to be loaded when the type--pcase-macroexpander function
is called, which then causes cl-lib to be loaded.  So an additional
(require 'cl-lib) would be redundant.  However, as I said, there are
nuances to autoloading that I haven't grokked yet, so I may be missing
something.