Test Switch Expression lowering when case is constant #169

mabbay · 2024-07-04T08:42:34Z

Test Switch Expression lowering when case is constant.

Progress

Change must not contain extraneous whitespace

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to f4c40ea9

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/169/head:pull/169
$ git checkout pull/169

Update a local copy of the PR:
$ git checkout pull/169
$ git pull https://git.openjdk.org/babylon.git pull/169/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 169

View PR using the GUI difftool:
$ git pr show -t 169

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/169.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-04T08:43:08Z

👋 Welcome back mabbay! A progress list of the required criteria for merging this PR into code-reflection will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-07-04T08:43:21Z

@mabbay This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

Test Switch Expression lowering when case is constant

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 14 new commits pushed to the code-reflection branch:

2ebde56: Hat cleanup heal example
0b38abc: Pom fixes for life and heal examples
cd2b955: Simplified hatrun.bash. All examples now .Main
c575329: Simplified injection of Pre/Post calls Mutate,Access and Escape
82cec67: Convey dirty java buffer status to native code
3365ad3: Hat initial attempt minimize copies
254e9cb: Improve sanity checking
01a60ee: Now longer allow cmake builds from hat root dir
f33330e: Some files had missing license headers
982090b: Create a new example for BlackScholes. Reintroduce Heal and Life examples
... and 4 more: https://git.openjdk.org/babylon/compare/0bc3ed9467e25875ce9d9201ce1e1fd5d499a5bc...code-reflection

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the code-reflection branch, type /integrate in a new comment.

mabbay · 2024-07-04T08:44:02Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java

-                return result != null ?
-                        coerce(result, tree.type, target) :
-                        null;
+                // there are cases where tree type is null


@mcimadamore what do you think about this ?

In principle, this method should only be called for expressions. And all expressions have some type. I see two cases where this is not true:

body of a switch label following a -> which can be either an expression, or a throw (the latter is a statement, so it has no type)

the body of a lambda expression, which can be either a JCBlock (no type) or a JCExpression

It might perhaps be better to cleanup this code by making sure that we have the following methods:

toValue(JCExpression, Type), like the one we have now

toValue(JCExpression) which delegates to toValue(expr, Type.noType)

toValue(JCStatement) which just call scan on the statement (no target type and no conversion here)

Then, clients will have to pay attention to which method to call. For instance, in the case of a switch label, we'd have to check whether the body of the label is a throw statement, and if so call the statement overload of toValue (w/o a target type).

mlbridge · 2024-07-04T08:48:57Z

Webrevs

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java

mcimadamore · 2024-07-22T09:09:47Z

test/jdk/java/lang/reflect/code/TestSwitchExpressionOp.java

@@ -15,112 +16,198 @@
 */
 public class TestSwitchExpressionOp {

-    // TODO more testing
-    //  cover cases where MatchException will be thrown
+    @Test


Shouldn't we add similar tests in the compiler (with the @IR annotation) to check that the model generated by javac contains the expected conversions?

You mean a test similar to caseConstantOtherKindsOfExpr ?

You mean a test similar to caseConstantOtherKindsOfExpr ?

At least something like that yes.

PaulSandoz

Looks good, i agree with Maurizio to add language tests comparing the models

mcimadamore · 2024-07-25T09:34:35Z

test/langtools/tools/javac/reflect/SwitchExpressionTest2.java

+                    }
+                    ()java.lang.String -> {
+                        %7 : java.lang.String = constant @"BAR";
+                        java.yield %7;


unrelated: but I found the distinction between yield and java.yield a bit confusing...

Indeed it is confusing. The core operation yield means to yield a value from a body, where as the extended operation java.yield models Java's yield statement. Perhaps rename the former to body.yield?

mcimadamore · 2024-07-25T09:44:47Z

test/langtools/tools/javac/reflect/SwitchExpressionTest2.java

+ * @build CodeReflectionTester
+ * @run main CodeReflectionTester SwitchExpressionTest2
+ */
+public class SwitchExpressionTest2 {


I'm a bit confused: the core of the fix is to add a conversion to the selector type is the case label is constant, right? But I don't see any conv node anywhere in the test (except for a single one where an explicit cast is used). Is the new code exercised in this test?

The core of the changes is to test switch expression when case is constant, in the process we found that we miss conversion of the constant to the the type of the selector expression, we added that. I agree that we should add tests that focus more on conversion.

caseConstantThrow shows how we are converting constant label to the selector expression's type.

I see - I was looking for convert but it's true that there are some unboxing conversions in there

mabbay · 2024-07-29T18:18:54Z

/integrate

openjdk · 2024-07-29T18:20:18Z

Going to push as commit b4e13d5.
Since your change was applied there have been 14 commits pushed to the code-reflection branch:

2ebde56: Hat cleanup heal example
0b38abc: Pom fixes for life and heal examples
cd2b955: Simplified hatrun.bash. All examples now .Main
c575329: Simplified injection of Pre/Post calls Mutate,Access and Escape
82cec67: Convey dirty java buffer status to native code
3365ad3: Hat initial attempt minimize copies
254e9cb: Improve sanity checking
01a60ee: Now longer allow cmake builds from hat root dir
f33330e: Some files had missing license headers
982090b: Create a new example for BlackScholes. Reintroduce Heal and Life examples
... and 4 more: https://git.openjdk.org/babylon/compare/0bc3ed9467e25875ce9d9201ce1e1fd5d499a5bc...code-reflection

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-29T18:20:28Z

@mabbay Pushed as commit b4e13d5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mabbay added 4 commits July 4, 2024 09:00

Remove switch expression lowering tests

9f8036c

Test switch expression lowering when case is constant

47075cb

Remove misleading comment

650e2f3

Merge branch 'code-reflection' into test-sw-lowering

8f133de

mabbay commented Jul 4, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 4, 2024

mabbay self-assigned this Jul 12, 2024

mabbay added 2 commits July 19, 2024 19:16

Merge branch 'code-reflection' into test-sw-lowering

f68c9c3

Create overloads of method toValue.

1457f1f

mabbay requested review from PaulSandoz and mcimadamore July 21, 2024 19:19

mcimadamore reviewed Jul 22, 2024

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java Show resolved Hide resolved

mcimadamore reviewed Jul 22, 2024

View reviewed changes

PaulSandoz reviewed Jul 22, 2024

View reviewed changes

PaulSandoz mentioned this pull request Jul 22, 2024

Test Switch Expression lowering when case is pattern #191

Closed

1 task

mabbay added 3 commits July 24, 2024 21:12

Add compiler tests of Switch Expression.

24191ca

Fix a typo

2a5023a

Write code models without location info

f4c40ea

mabbay requested a review from mcimadamore July 24, 2024 20:30

mcimadamore reviewed Jul 25, 2024

View reviewed changes

mcimadamore approved these changes Jul 26, 2024

View reviewed changes

Add tests that focus on conversion of case constant

1b69c9a

openjdk bot added the integrated Pull request has been integrated label Jul 29, 2024

openjdk bot closed this Jul 29, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 29, 2024

mabbay deleted the test-sw-lowering branch July 29, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Switch Expression lowering when case is constant #169

Test Switch Expression lowering when case is constant #169

mabbay commented Jul 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 4, 2024

openjdk bot commented Jul 4, 2024 •

edited

Loading

mabbay Jul 4, 2024

mcimadamore Jul 15, 2024

mlbridge bot commented Jul 4, 2024 •

edited

Loading

mcimadamore Jul 22, 2024

mabbay Jul 22, 2024

mcimadamore Jul 22, 2024

PaulSandoz left a comment

mcimadamore Jul 25, 2024

PaulSandoz Aug 2, 2024 •

edited

Loading

mcimadamore Jul 25, 2024

mabbay Jul 26, 2024

mabbay Jul 26, 2024

mcimadamore Jul 26, 2024

mabbay commented Jul 29, 2024

openjdk bot commented Jul 29, 2024

openjdk bot commented Jul 29, 2024

Test Switch Expression lowering when case is constant #169

Test Switch Expression lowering when case is constant #169

Conversation

mabbay commented Jul 4, 2024 • edited by openjdk bot Loading

Progress

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 4, 2024

openjdk bot commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Jul 4, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulSandoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulSandoz Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabbay commented Jul 29, 2024

openjdk bot commented Jul 29, 2024

openjdk bot commented Jul 29, 2024

mabbay commented Jul 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 4, 2024 •

edited

Loading

mlbridge bot commented Jul 4, 2024 •

edited

Loading

PaulSandoz Aug 2, 2024 •

edited

Loading