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.

Sign up for GitHub

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

Unify code that generates code model of switch statement and expression #220

Closed
wants to merge 3 commits into from
+151 −285
Closed

Unify code that generates code model of switch statement and expression #220

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a37833
Refactor
mabbay Sep 4, 2024
2be94f1
Refactor
mabbay Sep 4, 2024
784670c
Refactor
mabbay Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 39 additions & 53 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java
View file
Original file line number Diff line number Diff line change
@@ -1497,42 +1497,7 @@ public void visitSwitchExpression(JCTree.JCSwitchExpression tree) {
Type switchType = adaptBottom(tree.type);
FunctionType actionType = FunctionType.functionType(typeToTypeElement(switchType));

List<Body.Builder> bodies = new ArrayList<>();
Body.Builder defaultLabel = null;
Body.Builder defaultBody = null;

for (JCTree.JCCase c : tree.cases) {
Body.Builder caseLabel = visitCaseLabel(tree, target, c);

Body.Builder caseBody = visitCaseBody(tree, c);

if (c.labels.head instanceof JCTree.JCDefaultCaseLabel) {
defaultLabel = caseLabel;
defaultBody = caseBody;
} else {
bodies.add(caseLabel);
bodies.add(caseBody);
}
}

if (defaultLabel != null) {
bodies.add(defaultLabel);
bodies.add(defaultBody);
} else if (!tree.hasUnconditionalPattern) {
// label
pushBody(tree, FunctionType.VOID);
append(CoreOp._yield());
bodies.add(stack.body);
popBody();

// body
pushBody(tree, actionType);
append(CoreOp._throw(
append(CoreOp._new(FunctionType.functionType(JavaType.type(MatchException.class))))
));
bodies.add(stack.body);
popBody();
}
List<Body.Builder> bodies = visitSwitchStatAndExpr(tree, actionType, !tree.hasUnconditionalPattern);

result = append(ExtendedOp.switchExpression(actionType.returnType(), target, bodies));
}
@@ -1543,44 +1508,65 @@ public void visitSwitch(JCTree.JCSwitch tree) {

FunctionType actionType = FunctionType.VOID;

List<Body.Builder> bodies = visitSwitchStatAndExpr(tree, actionType,
tree.patternSwitch && !tree.hasUnconditionalPattern);

result = append(ExtendedOp.switchStatement(target, bodies));
}

private List<Body.Builder> visitSwitchStatAndExpr(JCTree tree, FunctionType caseBodyType, boolean isDefaultCaseNeeded) {
JCExpression selector;
List<JCTree.JCCase> cases;
if (tree instanceof JCTree.JCSwitch sw) {
selector = sw.selector;
cases = sw.cases;
} else if (tree instanceof JCTree.JCSwitchExpression sw) {
selector = sw.selector;
cases = sw.cases;
} else {
throw new IllegalStateException();
}

Value target = toValue(selector);

List<Body.Builder> bodies = new ArrayList<>();
Body.Builder defaultLabel = null;
Body.Builder defaultStatement = null;
Body.Builder defaultBody = null;

for (JCTree.JCCase c : tree.cases) {
Body.Builder caseBody = visitCaseLabel(tree, target, c);
for (JCTree.JCCase c : cases) {

Body.Builder statementBody = visitCaseBody(tree, c);
Body.Builder caseLabel = visitCaseLabel(tree, target, c);
Body.Builder caseBody = visitCaseBody(tree, c);

if (c.labels.head instanceof JCTree.JCDefaultCaseLabel) {
Copy link
Collaborator

@mcimadamore mcimadamore Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is optimal - can't we add all labels and bodies (even default ones) and then, after the loop, only add a synthetic default label if isDefaultCaseNeeded is set? Or, if there are cases where the flag is set, but there is already a real default label, can we maybe just unset the flag if we see a default label in the loop?

In other words, I don't see what adding the "regular" default body after the loop buys us.

Copy link
Member Author

@mabbay mabbay Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the regular default body to be in the last position.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This simplifies the lowering as example.

defaultLabel = caseBody;
defaultStatement = statementBody;
defaultLabel = caseLabel;
defaultBody = caseBody;
} else {
bodies.add(caseLabel);
bodies.add(caseBody);
bodies.add(statementBody);
}
}

if (defaultLabel != null) {
bodies.add(defaultLabel);
bodies.add(defaultStatement);
} else if (tree.patternSwitch && !tree.hasUnconditionalPattern) {
bodies.add(defaultBody);
} else if (isDefaultCaseNeeded) {
// label
pushBody(tree, FunctionType.VOID);
append(CoreOp._yield());
bodies.add(stack.body);
popBody();

// statement
pushBody(tree, actionType);
// body
pushBody(tree, caseBodyType);
append(CoreOp._throw(
append(CoreOp._new(FunctionType.functionType(JavaType.type(MatchException.class))))
));
bodies.add(stack.body);
popBody();
}

result = append(ExtendedOp.switchStatement(target, bodies));
return bodies;
}

private Body.Builder visitCaseLabel(JCTree tree, Value target, JCTree.JCCase c) {
@@ -1699,13 +1685,13 @@ private Body.Builder visitCaseLabel(JCTree tree, Value target, JCTree.JCCase c)
private Body.Builder visitCaseBody(JCTree tree, JCTree.JCCase c) {
Body.Builder body = null;

FunctionType actionType;
FunctionType caseBodyType;
Type yieldType = null;
if (tree instanceof JCTree.JCSwitch) {
actionType = FunctionType.VOID;
caseBodyType = FunctionType.VOID;
} else if (tree instanceof JCTree.JCSwitchExpression) {
Type switchType = adaptBottom(tree.type);
actionType = FunctionType.functionType(typeToTypeElement(switchType));
caseBodyType = FunctionType.functionType(typeToTypeElement(switchType));
yieldType = adaptBottom(tree.type);
} else {
throw new IllegalStateException();
@@ -1714,7 +1700,7 @@ private Body.Builder visitCaseBody(JCTree tree, JCTree.JCCase c) {
JCTree.JCCaseLabel headCl = c.labels.head;
switch (c.caseKind) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion, up to you. You can make this a switch expression yielding the body builder and directly return the result

return switch (c.caseKind) {
...

  try {
    yield stack.body;
  } finally {
    popBody();
  }
...

case RULE -> {
pushBody(c.body, actionType);
pushBody(c.body, caseBodyType);

if (c.body instanceof JCTree.JCExpression e) {
Value bodyVal = toValue(e, yieldType);
@@ -1738,7 +1724,7 @@ private Body.Builder visitCaseBody(JCTree tree, JCTree.JCCase c) {
case STATEMENT -> {
// @@@ Avoid nesting for a single block? Goes against "say what you see"
// boolean oneBlock = c.stats.size() == 1 && c.stats.head instanceof JCBlock;
pushBody(c, actionType);
pushBody(c, caseBodyType);

scan(c.stats);