RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]

[Jan Lahoda]

On Tue, 18 Apr 2023 13:43:45 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Jan Lahoda has updated the pull request incrementally with six additional commits since the last revision:
>> 
>>  - Fixing infinite loop where a binding pattern is replaced with a binding pattern for the same type.
>>  - Reflecting review comments.
>>  - Fixing exhaustiveness for unsealed supertype pattern.
>>  - No need to enable features after error reported.
>>  - SwitchBootstraps.typeSwitch should not initialize enum classes.
>>  - A prototype of avoiding enum initialization.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 915:
> 
>> 913: 
>> 914:                             for (PatternDescription pdOther : patterns) {
>> 915:                                 if (pdOther instanceof BindingPattern bpOther) {
> 
> This code redundantly checks a pattern against itself, which I think we can avoid.

Possibly, but the handling of the binding patterns is getting more complex, so I've opted to keep it uniform for now.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 927:
> 
>> 925:                                             it.remove();
>> 926:                                             reduces = true;
>> 927:                                             continue PERMITTED;
> 
> the label can be dropped here as there's no nested loop

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 953:
> 
>> 951:                     }
>> 952: 
>> 953:                     Set<PatternDescription> cleanedToRemove = new HashSet<>(toRemove);
> 
> Another way to do this would be to compute the intersection between `toRemove` and `toAdd` (e.g. with `retainAll`), and then remove the intersection from both sets.

The addition and removal of the same binding pattern should be completely avoided in the current version of the code.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1058:
> 
>> 1056:                                     for (int i = 0; i < rpOne.nested.length; i++) {
>> 1057:                                         if (i != mismatchingCandidate &&
>> 1058:                                             !exactlyMatches(rpOne.nested[i], rpOther.nested[i])) {
> 
> should `exactlyMatches` be the implementation of `equals` in the `PatternDescriptor` class?

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java line 847:
> 
>> 845:     /** Skip parens and return the enclosed expression
>> 846:      */
>> 847:     //XXX: remove??
> 
> What about this?

Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9

Thanks for the comment!

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172875982
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172876837
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172875031
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877099
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877541