On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing typo. A really nice feature, and a significant amount of work in javac. I mostly focused on the bootstrap and API aspects, and left some minor comments (most of which you can choose to apply or not as you see fit). I suspect the bootstrap might evolve as we get feedback and switch is enhanced with further forms of matching. But, at the moment it looks good. src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 87: > 85: * returns {@literal -1}. > 86: * <p> > 87: * the {@code target} is not {@code null}, then the method of the > call site Suggestion: * If the {@code target} is not {@code null}, then the method of the call site src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 92: > 90: * <ul> > 91: * <li>the element is of type {@code Class} and the target value > 92: * is a subtype of this {@code Class}; or</li> Suggestion: * <li>the element is of type {@code Class} that is assignable * from the target's class; or</li> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 162: > 160: return i; > 161: } else { > 162: if (label instanceof Integer constant) { Minor suggestion: use `else if` rather than nest src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 166: > 164: return i; > 165: } > 166: if (target instanceof Character input && > constant.intValue() == input.charValue()) { Use `else if` src/jdk.compiler/share/classes/com/sun/source/tree/CaseLabelTree.java line 31: > 29: > 30: /**A marker interface for {@code Tree}s that may be used as {@link > CaseTree} labels. > 31: * Suggestion: /** * A marker interface for {@code Tree}s that may be used as {@link CaseTree} labels. * src/jdk.compiler/share/classes/com/sun/source/tree/DefaultCaseLabelTree.java line 30: > 28: > 29: /** A case label that marks {@code default} in {@code case null, default}. > 30: * @since 17 Suggestion: /** * A case label that marks {@code default} in {@code case null, default}. * * @since 17 test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 55: > 53: catch (NoSuchMethodException | IllegalAccessException e) { > 54: throw new RuntimeException(e); > 55: } Suggestion: catch (ReflectiveOperationException e) { throw new AssertionError(e, "Should not happen"); } test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 73: > 71: } > 72: > 73: public void testTypes() throws Throwable { As a follow up issue consider adding tests for `null` values test/langtools/tools/javac/patterns/DisambiguateParenthesizedPattern.java line 72: > 70: SwitchTree st = (SwitchTree) > method.getBody().getStatements().get(0); > 71: CaseLabelTree label = st.getCases().get(0).getLabels().get(0); > 72: ExpressionType actualType = switch (label) { Should the test be careful of using a pattern match switch? ------------- Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3863
