bug#73725: Master: Wrong position for byte compiler warning message.

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

On Wed, Oct 23, 2024 at 09:27:12 -0400, Stefan Monnier wrote:
> LGTM.  Further nitpicks below,


>         Stefan


> > diff --git a/lisp/emacs-lisp/byte-opt.el b/lisp/emacs-lisp/byte-opt.el
> > index d8dbfa62bf9..5cfc3528492 100644
> > --- a/lisp/emacs-lisp/byte-opt.el
> > +++ b/lisp/emacs-lisp/byte-opt.el
> > @@ -509,8 +509,12 @@ byte-optimize-one-form
> >  (defun byte-optimize-form (form &optional for-effect)
> >    (while
> >        (progn
> > -        ;; First, optimize all sub-forms of this one.
> > -        (setq form (byte-optimize-form-code-walker form for-effect))
> > +        ;; First, optimize all sub-forms of this one.  Note that
> > +        ;; `byte-optimize-form-code-walker' sometimes changes the car of
> > +        ;; `form', hence the `macroexp-preserve-posification'.
> > +        (setq form
> > +              (macroexp-preserve-posification
> > +               form (byte-optimize-form-code-walker form for-effect)))

> We know it can change the `car`, that's not the question I think the
> comment should answer.  The comment should instead explain why
> `byte-optimize-form-code-walker` doesn't use
> `macroexp-preserve-posification` in those few places where it can change
> the `car`s.
> IIUC the answer is something like "because it was more work".

Well, we could argue for some time as to whether 5 items from 20 is
"few" or not.  But I've amended the comment to say the invocation to
m-p-posification is here for "source code economy".

As I think I've already said, the slowdown on a make bootstrap is around
0.5%, barely measureable without using perf.

> > @@ -524,7 +603,9 @@ macroexpand-all
> >  definitions to shadow the loaded ones for use in file byte-compilation."
> >    (let ((macroexpand-all-environment environment)
> >          (macroexp--dynvars macroexp--dynvars))
> > -    (macroexp--expand-all form)))
> > +    (macroexp-preserve-posification
> > +        form
> > +      (macroexp--expand-all form))))

> I missed this one earlier: why do we need this one?

Very good point.  I vaguely remember going through this
macro--expand-all when I was introducing the SWPs in the first place.
However, there's one point in the function where the SWP isn't
necessarily preserved, and that's near the end where compiler macros get
handled.  Here, we have less control over what the handler does.  So I
put an invocation of macroexp-preserve-posification into
macroexp--compiler-macro to fix this, and took the one out of
macroexpand-all as not needed, as you suggested.

> Doesn't `macroexp--expand-all` already take care of
> preserving positions?
> [ You don't need to answer here: better answer in the code 🙂  ]

I've done both!  But I'm now confident enough about the patch not to
send it to you for a third time.

Thanks for all the feedback.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).