Re: [Emacs-diffs] trunk r117002: Correctly treat progn contents as toplevel forms when byte compiling

[Daniel Colascione]

On 04/21/2014 08:09 AM, Stefan Monnier wrote:
>>   Correctly treat progn contents as toplevel forms when byte compiling
> 
> Your commit messages should be copies of the ChangeLog entry.
> 
> Could you describe the case(s) that this fixes?

See the testcases. Generally speaking, this change fixes situations
where the byte-compiler miscompiles code that contains (or generates)
top-level progns that define macros, then use them.

>> +  ;; Macroexpand (not macroexpand-all!)
> 
> That could be a problem.

Why? We macroexpand-all forms later when we actually compile or eval them.

>> form at toplevel in case it
>> +  ;; expands into a toplevel-equivalent `progn'.  See CLHS section
>> +  ;; 3.2.3.1, "Processing of Top Level Forms".
> 
> Note that Elisp is not Common-Lisp, so we don't always follow
> Common-Lisp's design decisions (although, we often do, as well).

Unless there's a good reason to depart from CL, we should follow CL's
approach to things. CL in a good sane default, and in this case, CL
specifies exactly the right behavior.

>> The semantics are very
>> +  ;; subtle: see test/automated/bytecomp-tests.el for interesting
>> +  ;; cases.
>> +  (setf form (macroexpand form byte-compile-macro-environment))
>> +  (if (eq (car-safe form) 'progn)
>> +      (cons 'progn
>> +            (mapcar (lambda (subform)
>> +                      (byte-compile-recurse-toplevel
>> +                       subform non-toplevel-case))
>> +                    (cdr form)))
>> +    (funcall non-toplevel-case form)))
> 
> `non-toplevel-case' is declared as optional, but here you call it
> without ensuring it's non-nil.  IOW it shouldn't be optional.

Err, yes.

>>  (defconst byte-compile-initial-macro-environment
>>    '(
>>      ;; (byte-compiler-options . (lambda (&rest forms)
>>      ;;                     (apply 'byte-compiler-options-handler forms)))
>>      (declare-function . byte-compile-macroexpand-declare-function)
>>      (eval-when-compile . (lambda (&rest body)
> 
> Oops, we have a bug here.  We should be using `(... ,(lambda ...

Well, a minor performance bug. Still worth fixing.

signature.asc
Description: OpenPGP digital signature