Fix #480 in LambdaLift #486
Conversation
Fix two errors, where the first masked the second: 1) Variables defined in a method are not free variables of that method. This was mispredicated before and caused liftedOwner to be updated to the class enclosing the method, thereby preventing any method that refers to a local parameter or symbol to be hoisted. Once this was fixed, methods were hoisted too far out. Test case in #480a, which takes code from Definitions. This was fixed by 2) markFree is updated to do an additional narrowLiftedOwner so that, if a free variable gets a proxy in an enclosing class, any inner classes and methods are kept within that class.
| * and the owner of `sym`. | ||
| * Return `true` if there is no class between `enclosure` and | ||
| * the owner of sym. | ||
| * and the owner of `sym`. Also, update lifted owner of `enclosure` so |
There was a problem hiding this comment.
there is a class between
enclosures owner and the owner ofsym
It is still no clear that method follows the contract. The method does not reffer to the sym.owner at all, instead it uses sym.enclosingClass.
I believe that this is an important method which needs a more verbose documentation, both for the contract and for the implementation.
There was a problem hiding this comment.
I'd better change lifted owner of enclosure to
value of liftedOwner(enclosure), to make it clear that the map is updated, not the symbol.
|
can you also have a look on #342? |
| if (enclosure.is(PackageClass)) enclosure | ||
| else markFree(sym, enclosure.skipConstructor.enclosure) | ||
| if (intermediate.exists) { | ||
| narrowLiftedOwner(enclosure, intermediate) |
There was a problem hiding this comment.
can you either add documentation to narrowLiftedOwner method, or add description what happens in both branches of this if?
|
@DarkDimius all reviewer comments should be addressed by now. I'll have a look at #342 independently. |
|
Now #432 should also be fixed. |
|
@DarkDimius Can you check which of the other lambda lift related issues are still open? |
|
Same applies to test name. |
|
Otherwise LGTM. It seems that we have no open issues with lambdaLift left. |
After lambdaLift, methods are no longer local to cosntructors, so their inSuperCall flag is reset.
Idents of lifted symbols become class members, need to carry the right reference with the right prefix as type.
DarkDimius
force-pushed
the
fix/lambdaLift
branch
from
687cbad to
0b4e4cb
Compare
Fix two errors, where the first masked the second:
and caused liftedOwner to be updated to the class enclosing the method, thereby preventing any method
that refers to a local parameter or symbol to be hoisted.
Once this was fixed, methods were hoisted too far out. Test case in #480a, which takes code from Definitions. This was fixed by
in an enclosing class, any inner classes and methods are kept within that class.
@review by @DarkDimius