* High degree of code duplication in instanceof pattern code generation

* Fixes #2021
eclipse-jdt · Feb 14, 2024 · 8bea15f · 8bea15f
1 parent 4398353
commit 8bea15f
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 51 deletions.
diff --git a/...t.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/InstanceOfExpression.java b/...t.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/InstanceOfExpression.java
@@ -104,54 +104,34 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
 @Override
 public void generateCode(BlockScope currentScope, CodeStream codeStream, boolean valueRequired) {
 
-	int pc = codeStream.position;
+	BranchLabel falseLabel = new BranchLabel(codeStream);
+	BranchLabel continueLabel = new BranchLabel(codeStream);
 
-	this.expression.generateCode(currentScope, codeStream, true);
-	if (this.secretExpressionValue != null) {
-		codeStream.store(this.secretExpressionValue, true);
-		codeStream.addVariable(this.secretExpressionValue);
-	}
-	codeStream.instance_of(this.type, this.type.resolvedType);
-	if (this.pattern != null) {
-		BranchLabel falseLabel = new BranchLabel(codeStream);
-		BranchLabel trueLabel = new BranchLabel(codeStream);
-		BranchLabel continueLabel = new BranchLabel(codeStream);
-		codeStream.ifeq(falseLabel);
+	generateOptimizedBoolean(currentScope, codeStream, null, falseLabel, valueRequired);
 
-		if (this.secretExpressionValue != null) {
-			codeStream.load(this.secretExpressionValue);
-			codeStream.removeVariable(this.secretExpressionValue);
-		} else {
-			this.expression.generateCode(currentScope, codeStream, true);
-		}
-		this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, falseLabel);
-
-		trueLabel.place();
+	if (valueRequired) {
 		codeStream.iconst_1();
 		codeStream.goto_(continueLabel);
-		falseLabel.place();
+	}
+	falseLabel.place();
+	/* We are generating a "thunk" of sorts now, that flow analysis has no clue about.
+	   so, we need to manage the live variables manually. Pattern bindings are not definitely
+	   assigned here as we are in instanceof false region.
+    */
+	if (this.pattern != null) {
 		for (LocalVariableBinding binding : this.pattern.bindingsWhenTrue()) {
 			binding.recordInitializationEndPC(codeStream.position);
 		}
+	}
+	if (valueRequired)
 		codeStream.iconst_0();
-		continueLabel.place();
 
-	}
-	if (valueRequired) {
-		codeStream.generateImplicitConversion(this.implicitConversion);
-	} else {
-		codeStream.pop();
-	}
-	codeStream.recordPositionsFrom(pc, this.sourceStart);
+	continueLabel.place();
+	codeStream.recordPositionsFrom(codeStream.position, this.sourceEnd);
 }
+
 @Override
 public void generateOptimizedBoolean(BlockScope currentScope, CodeStream codeStream, BranchLabel trueLabel, BranchLabel falseLabel, boolean valueRequired) {
-	// a label valued to nil means: by default we fall through the case...
-	// both nil means we leave the value on the stack
-	if (this.elementVariable == null && this.pattern == null) {
-		super.generateOptimizedBoolean(currentScope, codeStream, trueLabel, falseLabel, valueRequired);
-		return;
-	}
 
 	int pc = codeStream.position;
 
@@ -161,23 +141,23 @@ public void generateOptimizedBoolean(BlockScope currentScope, CodeStream codeStr
 		codeStream.addVariable(this.secretExpressionValue);
 	}
 
-	BranchLabel nextSibling = falseLabel != null ? falseLabel : new BranchLabel(codeStream);
+	BranchLabel internalFalseLabel = falseLabel != null ? falseLabel : new BranchLabel(codeStream);
 	codeStream.instance_of(this.type, this.type.resolvedType);
-	codeStream.ifeq(nextSibling);
-	if (this.secretExpressionValue != null) {
-		codeStream.load(this.secretExpressionValue);
-		codeStream.removeVariable(this.secretExpressionValue);
-	} else {
-		this.expression.generateCode(currentScope, codeStream, true);
+	codeStream.ifeq(internalFalseLabel);
+	if (this.pattern != null) {
+		if (this.secretExpressionValue != null) {
+			codeStream.load(this.secretExpressionValue);
+			codeStream.removeVariable(this.secretExpressionValue);
+		} else {
+			this.expression.generateCode(currentScope, codeStream, true);
+		}
+		this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, internalFalseLabel);
 	}
 
-	this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, nextSibling);
-
-	if (valueRequired) {
-		codeStream.generateImplicitConversion(this.implicitConversion);
-	} else {
+	if (!valueRequired) {
 		codeStream.pop();
 	}
+
 	codeStream.recordPositionsFrom(pc, this.sourceStart);
 
 	int position = codeStream.position;
@@ -197,8 +177,8 @@ public void generateOptimizedBoolean(BlockScope currentScope, CodeStream codeStr
 	}
 	codeStream.recordPositionsFrom(position, this.sourceEnd);
 
-	if (nextSibling != falseLabel)
-		nextSibling.place();
+	if (internalFalseLabel != falseLabel)
+		internalFalseLabel.place();
 }
 
 @Override

diff --git a/...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java b/...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java
@@ -988,7 +988,10 @@ private void generateCodePatternCaseEpilogue(CodeStream codeStream, int caseInde
 			Pattern pattern = (Pattern) caseStatement.constantExpressions[caseStatement.patternIndex];
 			pattern.elseTarget.place();
 			if (!pattern.isAlwaysTrue()) {
-				// at the trampoline here, pattern bindings are not definitely assigned.
+				/* We are generating a "thunk"/"trampoline" of sorts now, that flow analysis has no clue about.
+				   We need to manage the live variables manually. Pattern bindings are not definitely
+				   assigned here as we are in the else region.
+			    */
 				final LocalVariableBinding[] bindingsWhenTrue = pattern.bindingsWhenTrue();
 				Stream.of(bindingsWhenTrue).forEach(v->v.recordInitializationEndPC(codeStream.position));
 				codeStream.loadInt(this.nullProcessed ? caseIndex - 1 : caseIndex);