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

[Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

>> Is this needed?  I'd expect we need `macroexp-preserve-posification`
>> only at those places where an optimization returns a form whose `car` is
>> different from that of FORM.  So I expect this to happen in more
>> specific places inside byte-opt.el than here.
> Yes, it is needed.  byte-optimize-form-code-walker sometimes does return
> a form with a different car.  For example, a progn form with a single
> sub-form comes back without the progn.

This should usually be harmless since the subform should come with its own
position information.

> Even if there are several sub-forms, the code uses macroexp-progn to
> substitute a different progn symbol without the position.  I don't
> know why it does this.

This is more annoying, indeed.  Ideally, we'd move the
`macroexp-preserve-posification` to the place(s) where this remove+readd
`progn` happens.

> One of my ideas was to fix byte-optimize-form-code-walker by fixing each
> individual bit where the SWP got lost.  In the end I gave that up as too
> much work, and too difficult to test.

Hmm... I see.  We can leave for a later optimization of
`byte-optimize-form-code-walker`.

>> IOW, this deserves a clear comment explaining why we need it here,
>> probably with some kind of example.
> OK, I'll put one in, along the lines of the above, but less wordy.

Thanks.

> Good point.  The doc string of that function is a little clumsy, as it
> refers to the doc string of the next function macroexp--posify-form,

That part did not bother me.  These are internal functions so it's OK if
the docstring is a bit less "self contained".

> But I'll write somewhere that we modify "a single symbol", or something
> like that.

Thanks.

>> >  (defun macroexp-macroexpand (form env)
>> >    "Like `macroexpand' but checking obsolescence."
>> >    (let* ((macroexpand-all-environment env)
>> >           new-form)
>> > +    (macroexp-preserve-posification
>> > +     form
>> >      (while (not (eq form (setq new-form (macroexpand-1 form env))))
>> > +       (setq macroexpanded t)
>> >        (let ((fun (car-safe form)))
>> >          (setq form
>> >                (if (and fun (symbolp fun)
>
>> This `(setq macroexpanded t)` looks like some leftover code, at least
>> I couldn't find this var declared or used elsewhere.
>
> I remember removing it, but can't remember exactly why.  When I byte
> compile the code, I don't get an undeclared variable warning for it, for
> some reason.

[ Interesting.  I'll try to remember to track down this sucker later.  ]

>> > -(defun macroexpand-all (form &optional environment)
>> > +(defun macroexpand-all (form &optional environment keep-pos)
>> Any reason why we need this new argument?
>> Can't we just always try to preserve the positions?
> There are lots of calls to macroexpand-all (around 37), and I was
> concerned about possible accidental side effects in these.

I think we should assume that those potential side effects would more
likely be beneficial than harmful.

Another way to look at it is to look at the doc you provided:

    KEEP-POS, if non-nil, specifies that any symbol-with-position for
    FORM should be preserved, later to be usable by
    `byte-compile--warning-source-offset'.

Even when `keep-pos` is nil, `macroexpand-all` will preserve most of the
SWPs, so the doc is misleading.

> Also, always trying to preserve the position might slow down
> compilation, but I haven't measured that yet.

I think the calls where you currently ask for `keep-pos` account for the
vast majority of the time spent macro-expanding, so I'd be surprised if
doing it in all cases would make it measurably worse.


        Stefan