On Mon, 17 Apr 2023 11:33:56 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 879:
>
>> 877: }
>> 878: case TYPEVAR -> components(((TypeVar)
>> seltype).getUpperBound());
>> 879: default -> {
>
> The block here is redundant
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 890:
>
>> 888: * for the sealed supertype.
>> 889: */
>> 890: private List<PatternDescription> reduceBindingPatterns(Type
>> selectorType, List<PatternDescription> patterns) {
>
> This method doesn't seem to work for the following case:
>
>
> class Test {
> sealed interface I permits A, B, C { }
> sealed interface I3 permits I2 { }
> sealed interface I2 extends I3 permits B, C { }
> final class A implements I {}
> final class B implements I, I2 {}
> final class C implements I, I2 {}
>
> int m(I i) {
> return switch (i) {
> case A a -> 1;
> case I3 e -> 2;
> };
> }
> }
>
>
> There seems to be some ordering issue in the way we visit the patterns.
Hopefully fixed:
https://github.com/openjdk/jdk/pull/13074/commits/857980847e21aee0dee4665f19c1a8a54cff4973
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 901:
>
>> 899: Type clazzErasure =
>> types.erasure(clazz.type);
>> 900: if (components(selectorType).stream()
>> 901: .map(c ->
>> types.erasure(c))
>
> Suggestion:
>
> .map(Types::erasure)
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 914:
>
>> 912: permitted.remove(bpOne.type.tsym);
>> 913: bindings.append(it1.head);
>> 914: for (var it2 = it1.tail; it2.nonEmpty();
>> it2 = it2.tail) {
>
> This could be a for-each loop?
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 971:
>
>> 969: /* implementation note:
>> 970: * finding a sub-set of patterns that only differ in a
>> single
>> 971: * column is time consuming task, so this method speeds it
>> up by:
>
> Suggestion:
>
> * column is time-consuming task, so this method speeds it up by:
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 976:
>
>> 974: * -- group patterns by their hashs
>> 975: * -- in each such by-hash group, find sub-sets that only
>> differ in
>> 976: * the chosen column, and tcall reduceBindingPatterns
>> and reduceNestedPatterns
>
> Suggestion:
>
> * the chosen column, and call reduceBindingPatterns and
> reduceNestedPatterns
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 977:
>
>> 975: * -- in each such by-hash group, find sub-sets that only
>> differ in
>> 976: * the chosen column, and tcall reduceBindingPatterns
>> and reduceNestedPatterns
>> 977: * on patterns in the chosed column, as described above
>
> Suggestion:
>
> * on patterns in the chosen column, as described above
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 999:
>
>> 997: .collect(groupingBy(pd ->
>> pd.hashCode(mismatchingCandidateFin)));
>> 998: for (var candidates : groupByHashes.values()) {
>> 999: var candidatesArr = candidates.toArray(s -> new
>> RecordPattern[s]);
>
> Could this be an array constructor reference? E.g. `RecordPattern[]::new` ?
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1077:
>
>> 1075: */
>> 1076: private List<PatternDescription>
>> reduceRecordPatterns(List<PatternDescription> patterns) {
>> 1077: var newList = new ListBuffer<PatternDescription>();
>
> Maybe `newPatterns` would be a better name?
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1079:
>
>> 1077: var newList = new ListBuffer<PatternDescription>();
>> 1078: boolean modified = false;
>> 1079: for (var it = patterns; it.nonEmpty(); it = it.tail) {
>
> for-each loop here?
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1081:
>
>> 1079: for (var it = patterns; it.nonEmpty(); it = it.tail) {
>> 1080: if (it.head instanceof RecordPattern rpOne) {
>> 1081: PatternDescription nue =
>> reduceRecordPattern(rpOne);
>
> I find the name `nue` (here and elsewhere) particularly obscure.
Renamed `nue` to different names on this and other places:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1084:
>
>> 1082: if (nue != rpOne) {
>> 1083: newList.append(nue);
>> 1084: modified |= true;
>
> Suggestion:
>
> modified = true;
Fixed:
https://github.com/openjdk/jdk/pull/13074/commits/4a70b8d9710a21a7c230fe747af3f4f7ba6a388a
Thanks!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169908961
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169907961
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169910615
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169910699
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169908669
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169908754
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169908874
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169909065
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169910097
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169910379
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169909734
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169909809
