GNU bug report logs - #51982
Erroneous handling of local variables in byte-compiled nested lambdas

Previous Next

Full log

View this message in rfc822 format

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Paul Pogonyshev <pogonyshev <at> gmail.com>, 51982 <at> debbugs.gnu.org
Subject: bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas
Date: Wed, 01 Dec 2021 13:34:01 -0500

>> BTW, have you checked the impact on byte-code quality?
> With respect to these patches?

No I meant w.r.t removing `let*` (or `let` as the case may be).

> Yes: the B patch gives slightly better code because materialising the
> accessor (internal-get-closed-var N) is as cheap or cheaper than even
> a stack variable access. But the difference is small and since the
> case is rare it's probably insignificant.

I'm not worried at all about the performance of this corner-case.

>> Also, If mapping is of the form (car-safe SYMBOL) is `var` really the
>> correct answer?  Shouldn't it still be (cadr mapping)?
> Can there ever be a difference?

There's a big philosophical difference, yes.

>> Side note: I don't understand why we `(cons closedsym`, since that
>> `closedsym` can never appear in another binding (since it's fresh).
> Maybe it's to satisfy the invariant checked by the assertion at the top?

I don't think so because this one just checks that the
`cconv--remap-llv` was called where needed and did its job.

... [ goes and removes that `(cons closedsym` ] ...
... [ `make` ] ...

Oh, you're right!

>> I don't much like this `symbolp` test (which fundamentally seems to
>> be trying to recover the information about which branch of the `pcase`
>> we're coming from in `cconv--lifted-arg`).
> That's precisely what it is trying to do and no, I don't like it much either.

I think another way to do the patch B would be to replace `var` with
`lifted` right when we construct the (apply-partially ...) thingy
(i.e. in the :lambda-candidate part of the function), so those vars that
get remapped to `internal-get-closed-var` wouldn't even make their way
to `extend`.

> I don't see any alternative that is obviously better so I'm applying patch
> A. We can still go with B later on if we want; the changes are minor.

Good.

> Good comments, thank you very much!

[ I resent this implicit suggestion that I could ever write something less
  than a good comment.  ]


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.