On Fri, 17 Mar 2023 12:15:58 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
>
> - the pattern matching for switch and record patterns features are made
> final, together with updates to tests.
> - parenthesized patterns are removed.
> - qualified enum constants are supported for case labels.
>
> This change herein also includes removal record patterns in for each loop,
> which may be split into a separate PR in the future.
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
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.
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)
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?
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:
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 974:
> 972: * - group the patterns by their record class
> 973: * - for each column (nested pattern) do:
> 974: * -- group patterns by their hashs
Suggestion:
* -- group patterns by their hash
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
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
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` ?
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?
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?
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.
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;
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168552492
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168546493
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168560655
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168560880
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168551434
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168551577
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168551859
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168552121
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168553071
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168556508
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168557288
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168554679
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1168554833
