Fix nested pattern code generation #1816
Conversation
org.eclipse.jdt.core.compiler.batch/scripts/GenerateParserScript.class
Outdated
Show resolved
Hide resolved
datho7561
force-pushed
the
1804-record-pattern-value
branch
2 times, most recently
from
c9fa43f
to
0d2e7f1
Compare
|
It might be a good idea to also create a test for something like the following: public class Test{
record Box<T>(T t){}
public static void main(String[] args){
Box<Object> box = new Box<>(null);
boolean b = box instanceof Box(Object o);//true
System.out.println(b);
boolean c = box instanceof Box(String s);//false
System.out.println(c);
}
}
|
datho7561
force-pushed
the
1804-record-pattern-value
branch
3 times, most recently
from
b7d3b11
to
2d42344
Compare
datho7561
changed the title
|
@iloveeclipse @jarthana if you have time, could you please review this PR? |
|
@datho7561 I see that this pr fixes the #1796 issue as well when I checked. You can include that test case also here. Thanks! |
This PR doesn't seem to fix that bug from my testing. |
datho7561
force-pushed
the
1804-record-pattern-value
branch
from
2d42344
to
fddc825
Compare
Boolean assignments are working, but switch statements aren't. Fully removes unused pattern variables from stack map frame. Fixes eclipse-jdt#1804 Signed-off-by: David Thompson <davthomp@redhat.com>
Signed-off-by: David Thompson <davthomp@redhat.com>
Signed-off-by: David Thompson <davthomp@redhat.com>
Signed-off-by: David Thompson <davthomp@redhat.com>
Signed-off-by: David Thompson <davthomp@redhat.com>
datho7561
force-pushed
the
1804-record-pattern-value
branch
from
fddc825
to
ad08900
Compare
|
@datho7561 - This is my plan for this PR - I have assigned myself as a joint owner of this PR. As mentioned in #1517 (comment) I am investigating and possibly undertaking a overhaul of code generation for patterns. If you don't mind, I would subsume this PR as a part of the larger work. Let us not rush this please. What do you say ? |
| falseLabel.place(); | ||
| codeStream.iconst_0(); | ||
| continueLabel.place(); | ||
| } else if (this.elementVariable != null) { | ||
| BranchLabel actionLabel = new BranchLabel(codeStream); |
There was a problem hiding this comment.
This creates dead code - this.elementVariable will be null when(this.pattern == null)`
| public void fullWrapupGeneration(CodeStream codeStream) { | ||
| this.primaryPattern.fullWrapupGeneration(codeStream); | ||
| } | ||
| @Override | ||
| protected void generatePatternVariable(BlockScope currentScope, CodeStream codeStream, BranchLabel trueLabel, |
There was a problem hiding this comment.
Much water has flown under the bridge - the method wrapupGeneration doesn't exist anymore on master and it doesn't likely make sense to introduce this.
There was a problem hiding this comment.
I've rebased this PR against your changes in eclipse-jdtls#9 , I'll just need to replicate those changes here seems there are new changes to the Pattern classes since then
| @@ -117,15 +117,30 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream, boolean | |||
|
|
|||
| int pc = codeStream.position; | |||
|
|
|||
| if (this.elementVariable != null) { | |||
| if (this.elementVariable != null || this.pattern != null) { | |||
| addAssignment(currentScope, codeStream, this.secretInstanceOfPatternExpressionValue); | |||
| codeStream.load(this.secretInstanceOfPatternExpressionValue); | |||
| } else { | |||
| this.expression.generateCode(currentScope, codeStream, true); | |||
There was a problem hiding this comment.
you can simply say if (this.pattern != null)
| @@ -256,7 +263,7 @@ private static LocalVariableBinding getNewLocalVariableBinding(BlockScope scope, | |||
| // int delta = ((TypeBinding.equalsEquals(type, TypeBinding.LONG)) || | |||
| // (TypeBinding.equalsEquals(type, TypeBinding.DOUBLE))) ? | |||
| // 2 : 1; | |||
| l.resolvedPosition = scope.localIndex; | |||
| l.resolvedPosition = scope.startIndex + scope.localIndex; | |||
| return l; | |||
There was a problem hiding this comment.
This method is gone.
|
Interestingly I ran into this in another context and raised #1985 - I will include the tests from here and see if the changes in |
|
Looking through this code, I think it's worthwhile completely rewriting this fix, since the code has changed so much since when I wrote it. I'll make sure to leave the branch intact, since I think the code here might contain a hint towards the root cause. |
Fixes eclipse-jdt#1804 Supercedes eclipse-jdt#1816 Signed-off-by: David Thompson <davthomp@redhat.com>
Fixes eclipse-jdt#1804 Supercedes eclipse-jdt#1816 Signed-off-by: David Thompson <davthomp@redhat.com>
What it does
Fixes #1804
How to test
See the sample snippets that reproduce the error in the linked issue.
Author checklist