-
Notifications
You must be signed in to change notification settings - Fork 375
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
Implement Java 14 switch expressions #9981
Conversation
JValueLiteral caseValue = (JValueLiteral) expr; | ||
if (caseValue.getValueObj().equals(targetValue.getValueObj())) { | ||
matchingCase = caseStatement; | ||
break statements; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See about rewriting to avoid the labeled break, maybe a stream/optional operation?
Nightly build is passing https://github.com/niloc132/gwt/actions/runs/9978594294 |
I assume it is ready for review or do you want to address your second code comment first? |
I'd love some review yes, so we can get this shipped ASAP. I'm trying to finish up the closer-to-merging PRs, give people a chance to go over this, if interested, as its not as small as I had hoped. |
dev/core/src/com/google/gwt/dev/jjs/impl/gflow/cfg/CfgBuilder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to look good except that empty method in CfgBuilder. It is a blind spot now for the compiler analysis, right?
dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
Outdated
Show resolved
Hide resolved
@@ -645,6 +627,16 @@ public boolean visit(JStatement x, Context ctx) { | |||
throw new UnsupportedNodeException(x.getClass().toString()); | |||
} | |||
|
|||
@Override | |||
public boolean visit(JSwitchExpression x, Context ctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering about the consequences given that JSwitchStatement does quite a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlikely to matter in a meaningful way.
CfgBuilder is wrapped in DataflowOptimizer
, which itself is wrapped in a compiler flag, and warns if it is run at all:
gwt/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
Lines 1450 to 1455 in 7f42724
if (options.shouldOptimizeDataflow()) { | |
logger.log(TreeLogger.Type.WARN, | |
"Unsafe dataflow optimization enabled, disable with -XdisableOptimizeDataflow."); | |
// Just run it once, because it is very time consuming | |
allOptimizerStats.add(DataflowOptimizer.exec(jprogram)); | |
} |
The option is disabled by default:
private boolean optimizeDataflow = false; |
And while a compiler flag exists, its only purpose is to disable it:
gwt/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerDeprecatedOptimizeDataflow.java
Lines 27 to 52 in 7f42724
public ArgHandlerDeprecatedOptimizeDataflow(OptionOptimizeDataflow option) { | |
this.option = option; | |
addTagValue("-XdisableOptimizeDataflow", false); | |
} | |
@Override | |
public String getPurposeSnippet() { | |
return "Analyze and optimize dataflow."; | |
} | |
@Override | |
public String getLabel() { | |
return "optimizeDataflow"; | |
} | |
@Override | |
public boolean isUndocumented() { | |
return true; | |
} | |
@Override | |
public boolean setFlag(boolean value) { | |
option.setOptimizeDataflow(value); | |
return true; | |
} |
I'm not clear how a normal user can turn it on without a fork or a custom build.
I still updated tests to reduce the risk of anything bad happening here for downstream users - but this would only negatively impact new switch-expr usage, and would not deoptimize existing switch statements (as the tests confirm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I just found ea7228a
So maybe we should officially deprecate it for removal (or fix its bugs and keep it).
d8a11f0
to
7ca5ba9
Compare
Java switch expressions are implemented in JS as a function wrapped around a switch block, with
return
used foryield
, and multi-valuedcase
statements split into multiple entries. Most other optimizations that work on a switch statement at this time will not apply to switch expressions, these can be re-added at a later time, but other optimizations that have some specialization for switch statements have been tested or updated to handle switch expressions.JEP https://openjdk.org/jeps/361
Fixes #9878