Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roll back gratuitous changes to !instanceof code generation in the absence of pattern bindings #2098

Closed
srikanth-sankaran opened this issue Mar 5, 2024 · 0 comments · Fixed by #2099
Assignees

Comments

@srikanth-sankaran
Copy link
Contributor

eclipse-platform/eclipse.platform.releng.aggregator#1875 brings to light a case of avoidable code generation change brought in by #2011 in particular by #2021

While the new code emitted is correct, it does result in large number of comparator errors - it can be impossible to study and verify/certify that all differences are kosher.

Also, the new sequence is less optimal and avoidably so. Here is a pertinent comment from org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.generateOptimizedBoolean(BlockScope, CodeStream, BranchLabel, BranchLabel, boolean) :

     /* A label valued to null is supposed to mean: by default we fall through the case ...
	   Both null means no "optimization" and we leave the value on the stack
	   But we have trouble when
	        this.pattern != null && trueLabel != null && falseLabel == null

	   In this case, since we have no control over placement of the trueLabel, we won't know where to emit the pattern binding code.
	   So what we do is always emit ifeq even when optimization would call for ifne and treat the whole "blob" of pattern matching
	   code as part of the predicate. if you think about long and hard you can convince yourself this is semantically correct.
	*/

While the comment explains why some optimizations have to sacrificed - that is only when pattern bindings are declared and we can restore original optimized sequence otherwise.

@srikanth-sankaran srikanth-sankaran self-assigned this Mar 5, 2024
srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Mar 5, 2024
srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Mar 5, 2024
srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Mar 6, 2024
srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Mar 6, 2024
srikanth-sankaran added a commit to srikanth-sankaran/eclipse.jdt.core that referenced this issue Mar 6, 2024
srikanth-sankaran added a commit that referenced this issue Mar 6, 2024
…rn bindings is not gratuitously altered (#2099)

* Fixes #2098
robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Jul 18, 2024
gayanper pushed a commit to gayanper/eclipse.jdt.core that referenced this issue Sep 7, 2024
snjeza pushed a commit to snjeza/eclipse.jdt.core that referenced this issue Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant