[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#51982: Erroneous handling of local variables in byte-compiled nested
From: |
Mattias Engdegård |
Subject: |
bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas |
Date: |
Tue, 30 Nov 2021 18:01:59 +0100 |
30 nov. 2021 kl. 15.12 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> Can we avoid this duplication by moving that code to a separate function?
I extracted a big part of the code into a common function but left the free
variable access and mutation outside. (Really want to get rid of `let*`!)
> These two tests are identical aren't they?
No, they exercise different code paths (let and let*).
> Also, can we change the
> (setq x x) into something like (setq x (list x x)) and avoid using the
> same `b` value for both `x` vars, so as to catch more potential errors?
Yes, thank you, it was an editing mistake. Fixed.
> Looks good (better than patch A).
And here I was prepared to apply patch A since it's slightly more conservative
and it seems to be a rare problem anyway.
I've now split the patches in a more sensible (and easily reviewed) way: the
first corresponds to patch A, and the second is the diff to B. Take a second
look before making up your mind.
> You say "On the other hand, patch B does abuse the cconv data structures
> a little (but it works!)" so the code should say something about
> this abuse. A least I failed to see where the abuse lies.
There are comments and doc strings such as
EXTEND is a list of variables which might need to be accessed even from places
where they are shadowed, because some part of ENV causes them to be used at
places where they originally did not directly appear.
but with the B patch we put things into `extend` that are not strictly
variables but (international-get-closed-var N).
Similarly, `env` has entries like (VAR . (apply-partially F ARG1 ARG2 ..))
where the ARGi are always treated as variables but now they can be access forms
as well.
I suppose it doesn't matter much. There is an assertion at the very top of
`cconv-convert` which compares the elements by `eq` but it seems to work all
right...
Thanks for the review – new patches attached.
0001-Fix-closure-conversion-of-shadowed-captured-lambda-l.patch
Description: Binary data
0002-Improved-closure-conversion-of-shadowed-captured-lam.patch
Description: Binary data
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, (continued)
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Michael Heerdegen, 2021/11/21
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Mattias Engdegård, 2021/11/21
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Michael Heerdegen, 2021/11/22
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Mattias Engdegård, 2021/11/22
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Mattias Engdegård, 2021/11/22
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Stefan Monnier, 2021/11/30
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas,
Mattias Engdegård <=
- bug#51982: Erroneous handling of local variables in byte-compiled nested lambdas, Stefan Monnier, 2021/11/30