On Tue, 18 Apr 2023 13:43:45 GMT, Maurizio Cimadamore <mcimadam...@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
