Fix Bug in Painless Assignment#18379
Conversation
|
Lets get @uschindler to look. I do like the little bytecode assert in the test for now :) |
|
Looks OK. Sorry for the issue :-) |
| if (last instanceof ADefLink) { | ||
| final ADefLink lastDef = (ADefLink) last; | ||
| expression.analyze(settings, definition, variables); | ||
| // TODO: does it make more sense to just re-use last.after instead of using storeValueType? |
There was a problem hiding this comment.
I had something similar before, maybe revert back to using ALink.after for that - I was not sure if the operand should always have the same type like the after value. In any case I would keep the superclass ADefLink to make them easier to "detect" in those if statemments, although the ADefLink class is empty afterwards.
There was a problem hiding this comment.
@uschindler No worries about the issue. Definitely a somewhat strange corner case :) I agree on keeping it as a marker. It's very clean and clear way to identify these special cases. Do you think it makes more sense to have it as an interface once storeValueType is removed?
There was a problem hiding this comment.
"marker" interface is also fine. I made it as a class for the special cases we might introduce when we have more "def" types propagation. For now interface is also fine. It would spare to repeat the ctor!
There was a problem hiding this comment.
Okay cool. I'll make it an interface for now, and we can convert it to a class again as needed.
Yes, very cool. Maybe add some utility method to the ScriptTester base class to do those asserts. |
|
@uschindler Thank you for the feedback. Updated the changes a bit based on your feedback. Everything look okay? |
|
LGTM! |
|
@uschindler Thanks for the review! |
Added some tests for Def optimization and found a minor bug in assignment under a rare corner case.