Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 16:54:20 GMT, Jan Lahoda wrote: >> could you explain a bit more the relation between `lazyDoEnumSwitch` and >> `doEnumSwitch` because my understanding is that `lazyDoEnumSwitch` is >> invoking `doEnumSwitch` so to me whenever `lazyDoEnumSwitch` is invoked >> there will be an immediate invocation of `doEnumSwitch` that's why I thought >> they could be folded together > > So, when the `enumSwitch` bootstrap is called, the enum class may or may not > be initialized. So, to avoid initialization, we only do lightweight checks, > and construct a `MutableCallSite` with `lazyDoEnumSwitch` as a target. > > When the `CallSite` is invoked with an actual selector value, two options may > arise: > a) the selector value is null. The `lazyDoEnumSwitch` itself returns -1. > b) the selector value is not null. The enum class must already be initialized > (I believe). So, we create (hopefully) better structure to do the > categorization, based on `doEnumSwitch`, and set that into the `CallSite`. > Further calls will use this new `doEnumSwitch` based categorization directly, > without calling `lazyDoEnumSwitch`. The current call will manually delegate > to the `doEnumSwitch` version, but that should only happen "once" (or a small > number of times, due to running in different threads, etc.). I see, thanks for the explanation, this is nice - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172949033
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 17:08:10 GMT, Jan Lahoda wrote: >> That's in my upcoming update. > > The startIndex checks are part of this change: > https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9 > > Thanks for the comment! Oh I see, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172946111
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 13:43:45 GMT, Maurizio Cimadamore
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 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
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 14:31:46 GMT, Maurizio Cimadamore wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1021: >> >>> 1019: *on patterns in the chosen column, as described above >>> 1020: */ >>> 1021: var grouppedPerRecordClass = >> >> Suggestion: >> >> var groupedPerRecordClass = > > or `patternsByRecordClass`, or `groupByRecordClass` (the latter would be > consistent with `groupByHash` which is used below) 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_r1172876875
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 14:17:02 GMT, Maurizio Cimadamore
wrote:
>> I've also found an infinite loop with this:
>>
>>
>> class Test {
>> sealed interface I0 permits I1, I2 { }
>> sealed interface I00 permits I1, I2 { }
>>
>> sealed interface I1 extends I0, I00 permits B, C { }
>> sealed interface I2 extends I0, I00 permits B, C { }
>>
>> static final class B implements I1, I2 { }
>> static final class C implements I1, I2 { }
>>
>> int test(Object o) {
>> return switch (o) {
>> case B c -> 2;
>> case C d -> 3;
>> };
>> }
>> }
>
>> I've also found an infinite loop with this:
>
> I believe this has to do with the fact that the list of pattern is not a set
> - so we can end up adding the same pattern descriptions over and over.
Thanks for the testcase!
I've tried to fix this by (avoiding to add redundant binding patterns):
https://github.com/openjdk/jdk/pull/13074/commits/7e6ed619d89784c49a96984c0f74176a9e0bcf63
Sets might be a good move eventually, but so far it seemed to hide problems,
which quite probably needed to be solved anyway, so I kept the list impl for
now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172874262
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 16:40:31 GMT, Jan Lahoda wrote: >> and regarding checking that `startIndex` should be `>= 0`? shouldn't we >> check for that? > > That's in my upcoming update. The startIndex checks are part of this change: https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9 Thanks for the comment! - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877989
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 20:24:33 GMT, Vicente Romero 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 94:
>
>> 92: * the element is of type {@code String} or {@code Integer} and
>> 93: * equals to the target.
>> 94: * the element is of type {@code EnumDesc}, that describes a
>> constant that is
>
> I think that at ~line 76, up in this same javadoc you need to add a reference
> to EnumDesc in the general description. Where it says:
>
>
> The static arguments are an array of case labels which must be non-null and
> of type
> {@code String} or {@code Integer} or {@code Class}.
>
> also around line 107, where it says:
>
> @param labels case labels - {@code String} and {@code Integer} constants
>and {@code Class} instances, in any combination
Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9
Thanks for the comment!
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 263:
>
>> 261: private static > void validateEnumLabel(Class
>> enumClassTemplate, Object label) {
>> 262: if (label == null) {
>> 263: throw new IllegalArgumentException("null label found");
>
> I think that this one is not hit by any test
Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9
Thanks for the comment!
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 271:
>
>> 269:", expected the
>> provided enum class: " + enumClassTemplate);
>> 270: }
>> 271: } else if (labelClass == String.class) {
>
> I think that if the condition is changed to `labelClass != String.class` then
> `throw` we can drop an `else` branch
Should be resolved by:
https://github.com/openjdk/jdk/pull/13074/commits/bb59254e409821c9b14287c73a35ebe2dd2cbda9
Thanks for the comment!
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 274:
>
>> 272: //OK
>> 273: } else {
>> 274: throw new IllegalArgumentException("label with illegal type
>> found: " + labelClass +
>
> I think that this code is not being tested by any test
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/Attr.java line 201:
>
>> 199: /** Are unconditional patterns in instanceof allowed
>> 200: */
>> 201: private boolean allowUnconditionalPatternsInstanceOf;
>
> so I guess this field can be made final now
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 909:
>
>> 907:
>> 908: Set permitted =
>> allPermittedSubTypes(clazz, csym -> {
>> 909: Type instantiated =
>> infer.instantiatePatternType(selectorType, csym);
>
> for some cases, when there are no type parameters, this invocation is a
> no-op, do we really need inference at this point?
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_r1172877205
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877482
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877294
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172877401
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172878133
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172878329
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 16:31:33 GMT, Vicente Romero wrote: >> yep I found a similar test case triggering this code > > could you explain a bit more the relation between `lazyDoEnumSwitch` and > `doEnumSwitch` because my understanding is that `lazyDoEnumSwitch` is > invoking `doEnumSwitch` so to me whenever `lazyDoEnumSwitch` is invoked there > will be an immediate invocation of `doEnumSwitch` that's why I thought they > could be folded together So, when the `enumSwitch` bootstrap is called, the enum class may or may not be initialized. So, to avoid initialization, we only do lightweight checks, and construct a `MutableCallSite` with `lazyDoEnumSwitch` as a target. When the `CallSite` is invoked with an actual selector value, two options may arise: a) the selector value is null. The `lazyDoEnumSwitch` itself returns -1. b) the selector value is not null. The enum class must already be initialized (I believe). So, we create (hopefully) better structure to do the categorization, based on `doEnumSwitch`, and set that into the `CallSite`. Further calls will use this new `doEnumSwitch` based categorization directly, without calling `lazyDoEnumSwitch`. The current call will manually delegate to the `doEnumSwitch` version, but that should only happen "once" (or a small number of times, due to running in different threads, etc.). - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172863795
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 16:32:33 GMT, Vicente Romero wrote: >> I see, thanks > > and regarding checking that `startIndex` should be `>= 0`? shouldn't we check > for that? That's in my upcoming update. - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172848770
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 14:07:01 GMT, Vicente Romero wrote:
>> The intent here is to avoid eager initialization of enums - so, until the
>> enum is initialized, `lazyDoEnumSwitch` will be used (which, by itself,
>> won't compute any results, except the result for `null`), which is then
>> replaced with `doEnumSwitch` using the enum constants directly.
>>
>> The enum switches support patterns as well, so something like:
>>
>> enum E {A;}
>> E e = ...;
>> switch (e) {
>> case E ee -> {}
>> }
>>
>> is valid, and should trigger the part with Class labels.
>
> yep I found a similar test case triggering this code
could you explain a bit more the relation between `lazyDoEnumSwitch` and
`doEnumSwitch` because my understanding is that `lazyDoEnumSwitch` is invoking
`doEnumSwitch` so to me whenever `lazyDoEnumSwitch` is invoked there will be an
immediate invocation of `doEnumSwitch` that's why I thought they could be
folded together
>> `startIndex` may be non-0 when there are guards in the switch. In the case
>> of the `enumSwitch`, something like:
>>
>> enum E {A, B;}
>> E e = E.B;
>> switch (e) {
>> case E ee when ee == E.A -> {}
>> case E ee -> {}
>> }
>>
>>
>> the method will be called twice, one with `startIndex == 0` and once with
>> `startIndex == 1`, after the guard fails/returns `false`.
>
> I see, thanks
and regarding checking that `startIndex` should be `>= 0`? shouldn't we check
for that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172839418
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172840817
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 13:41:05 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 279:
>>
>>> 277: }
>>> 278:
>>> 279: private static int lazyDoEnumSwitch(Enum target, int
>>> startIndex, Object[] labels, MethodHandles.Lookup lookup, Class
>>> enumClass, MutableCallSite callSite) throws Throwable {
>>
>> out of curiosity, under what conditions the `startIndex` would be different
>> from `0`? also shouldn't we check that `startIndex` is `>= 0`?
>
> `startIndex` may be non-0 when there are guards in the switch. In the case of
> the `enumSwitch`, something like:
>
> enum E {A, B;}
> E e = E.B;
> switch (e) {
> case E ee when ee == E.A -> {}
> case E ee -> {}
> }
>
>
> the method will be called twice, one with `startIndex == 0` and once with
> `startIndex == 1`, after the guard fails/returns `false`.
I see, thanks
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 279:
>>
>>> 277: }
>>> 278:
>>> 279: private static int lazyDoEnumSwitch(Enum target, int
>>> startIndex, Object[] labels, MethodHandles.Lookup lookup, Class
>>> enumClass, MutableCallSite callSite) throws Throwable {
>>
>> can `doEnumSwitch` be folded into `lazyDoEnumSwitch`? just a suggestion, I'm
>> OK with either way just that now it is not clear that we need two methods
>> here. Also in `doEnumSwitch` and out of curiosity what example of user code
>> could hit this section:
>>
>>
>> if (label instanceof Class c) {
>> if (c.isAssignableFrom(targetClass))
>> return i;
>> }
>>
>> EDIT: nvm I found one example
>
> The intent here is to avoid eager initialization of enums - so, until the
> enum is initialized, `lazyDoEnumSwitch` will be used (which, by itself, won't
> compute any results, except the result for `null`), which is then replaced
> with `doEnumSwitch` using the enum constants directly.
>
> The enum switches support patterns as well, so something like:
>
> enum E {A;}
> E e = ...;
> switch (e) {
> case E ee -> {}
> }
>
> is valid, and should trigger the part with Class labels.
yep I found a similar test case triggering this code
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172646957
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172647746
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Wed, 19 Apr 2023 16:45:06 GMT, Vicente Romero 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 279:
>
>> 277: }
>> 278:
>> 279: private static int lazyDoEnumSwitch(Enum target, int startIndex,
>> Object[] labels, MethodHandles.Lookup lookup, Class enumClass,
>> MutableCallSite callSite) throws Throwable {
>
> can `doEnumSwitch` be folded into `lazyDoEnumSwitch`? just a suggestion, I'm
> OK with either way just that now it is not clear that we need two methods
> here. Also in `doEnumSwitch` and out of curiosity what example of user code
> could hit this section:
>
>
> if (label instanceof Class c) {
> if (c.isAssignableFrom(targetClass))
> return i;
> }
>
> EDIT: nvm I found one example
The intent here is to avoid eager initialization of enums - so, until the enum
is initialized, `lazyDoEnumSwitch` will be used (which, by itself, won't
compute any results, except the result for `null`), which is then replaced with
`doEnumSwitch` using the enum constants directly.
The enum switches support patterns as well, so something like:
enum E {A;}
E e = ...;
switch (e) {
case E ee -> {}
}
is valid, and should trigger the part with Class labels.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172623143
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Thu, 20 Apr 2023 13:33:17 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 170:
>>
>>> 168: } else if (label instanceof EnumDesc enumDesc) {
>>> 169: if (target.getClass().isEnum() &&
>>> 170: ((Enum)
>>> target).describeConstable().stream().anyMatch(d -> d.equals(enumDesc))) {
>>
>> probably a matter of style but isn't it a bit too much to use streams here?
>> there will be always only one element right?
>
> Technically, one element, but the type is optional, so formally could be no
> elements. I could use `.get()`, but that feels a bit ugly and against
> optional?
yep I know, up to you I'm not strong on this, I guess I was thinking about
performance but probably not using streams won't make a difference in this case
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172609916
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Wed, 19 Apr 2023 15:36:08 GMT, Vicente Romero 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 279:
>
>> 277: }
>> 278:
>> 279: private static int lazyDoEnumSwitch(Enum target, int startIndex,
>> Object[] labels, MethodHandles.Lookup lookup, Class enumClass,
>> MutableCallSite callSite) throws Throwable {
>
> out of curiosity, under what conditions the `startIndex` would be different
> from `0`? also shouldn't we check that `startIndex` is `>= 0`?
`startIndex` may be non-0 when there are guards in the switch. In the case of
the `enumSwitch`, something like:
enum E {A, B;}
E e = E.B;
switch (e) {
case E ee when ee == E.A -> {}
case E ee -> {}
}
the method will be called twice, one with `startIndex == 0` and once with
`startIndex == 1`, after the guard fails/returns `false`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172609212
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Wed, 19 Apr 2023 04:06:00 GMT, Vicente Romero 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 170:
>
>> 168: } else if (label instanceof EnumDesc enumDesc) {
>> 169: if (target.getClass().isEnum() &&
>> 170: ((Enum)
>> target).describeConstable().stream().anyMatch(d -> d.equals(enumDesc))) {
>
> probably a matter of style but isn't it a bit too much to use streams here?
> there will be always only one element right?
Technically, one element, but the type is optional, so formally could be no
elements. I could use `.get()`, but that feels a bit ugly and against optional?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1172599496
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/Attr.java line 201:
> 199: /** Are unconditional patterns in instanceof allowed
> 200: */
> 201: private boolean allowUnconditionalPatternsInstanceOf;
so I guess this field can be made final now
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 909:
> 907:
> 908: Set permitted =
> allPermittedSubTypes(clazz, csym -> {
> 909: Type instantiated =
> infer.instantiatePatternType(selectorType, csym);
for some cases, when there are no type parameters, this invocation is a no-op,
do we really need inference at this point?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1171830887
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1171936405
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 279:
> 277: }
> 278:
> 279: private static int lazyDoEnumSwitch(Enum target, int startIndex,
> Object[] labels, MethodHandles.Lookup lookup, Class enumClass,
> MutableCallSite callSite) throws Throwable {
can `doEnumSwitch` be folded into `lazyDoEnumSwitch`? just a suggestion, I'm OK
with either way just that now it is not clear that we need two methods here.
Also in `doEnumSwitch` and out of curiosity what example of user code could hit
this section:
if (label instanceof Class c) {
if (c.isAssignableFrom(targetClass))
return i;
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1171605248
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 279:
> 277: }
> 278:
> 279: private static int lazyDoEnumSwitch(Enum target, int startIndex,
> Object[] labels, MethodHandles.Lookup lookup, Class enumClass,
> MutableCallSite callSite) throws Throwable {
out of curiosity, under what conditions the `startIndex` would be different
from `0`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1171523963
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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. > > 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/tree/TreeInfo.java line 847: > 845: /** Skip parens and return the enclosed expression > 846: */ > 847: //XXX: remove?? What about this? - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1171141708
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 263:
> 261: private static > void validateEnumLabel(Class
> enumClassTemplate, Object label) {
> 262: if (label == null) {
> 263: throw new IllegalArgumentException("null label found");
I think that this one is not hit by any test
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 274:
> 272: //OK
> 273: } else {
> 274: throw new IllegalArgumentException("label with illegal type
> found: " + labelClass +
I think that this code is not being tested by any test
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170788337
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170786847
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 170:
> 168: } else if (label instanceof EnumDesc enumDesc) {
> 169: if (target.getClass().isEnum() &&
> 170: ((Enum)
> target).describeConstable().stream().anyMatch(d -> d.equals(enumDesc))) {
probably a matter of style but isn't it a bit too much to use streams here?
there will be always only one element right?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170784311
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 271:
> 269:", expected the
> provided enum class: " + enumClassTemplate);
> 270: }
> 271: } else if (labelClass == String.class) {
I think that if the condition is changed to `labelClass != String.class` then
`throw` we can drop an `else` branch
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170770395
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 149:
> 147: throw new IllegalArgumentException("label with illegal type
> found: " + label.getClass());
> 148: }
> 149: }
side: are there any cases for which the `startIndex` argument passed to method
`doTypeSwitch` is not `0`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170618764
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 94:
> 92: * the element is of type {@code String} or {@code Integer} and
> 93: * equals to the target.
> 94: * the element is of type {@code EnumDesc}, that describes a
> constant that is
I think that at ~line 76, up in this same javadoc you need to add a reference
to EnumDesc in the general description. Where it says:
The static arguments are an array of case labels which must be non-null and of
type
{@code String} or {@code Integer} or {@code Class}.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170531849
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 14:31:30 GMT, Maurizio Cimadamore 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 1021: > >> 1019: *on patterns in the chosen column, as described above >> 1020: */ >> 1021: var grouppedPerRecordClass = > > Suggestion: > > var groupedPerRecordClass = or `patternsByRecordClass`, or `groupByRecordClass` (the latter would be consistent with `groupByHash` which is used below) - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170131872
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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 1021:
> 1019: *on patterns in the chosen column, as described above
> 1020: */
> 1021: var grouppedPerRecordClass =
Suggestion:
var groupedPerRecordClass =
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?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170131443
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170135800
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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. > > 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 927: > 925: it.remove(); > 926: reduces = true; > 927: continue PERMITTED; the label can be dropped here as there's no nested loop - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170127214
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 13:54:52 GMT, Maurizio Cimadamore wrote: > I've also found an infinite loop with this: I believe this has to do with the fact that the list of pattern is not a set - so we can end up adding the same pattern descriptions over and over. - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170110465
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 13:43:17 GMT, Maurizio Cimadamore
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 812:
>
>> 810: if (l instanceof JCPatternCaseLabel patternLabel) {
>> 811: for (Type component :
>> components(selector.type)) {
>> 812: patterns =
>> patterns.prepend(PatternDescription.from(types, component,
>> patternLabel.pat));
>
> I noted that this code ends up adding redundant pattern descriptions to the
> list - for instance:
>
>
> class Test {
> sealed interface I1 permits B, C { }
> sealed interface I2 permits B, C { }
>
> static final class B implements I1, I2 { }
> static final class C implements I1, I2 { }
>
> int test(Z z) {
> return switch (z) {
> case B c -> 2;
> case C d -> 3;
> };
> }
> }
>
>
> In this case the list ends up with 6 elements, [ B, B, B, C, C, C ]. Given
> that the complexity of the algorithm depends on the number of patterns in the
> list, it would probably be better to use a set here and try to make the list
> as small as possible from early on.
I've also found an infinite loop with this:
class Test {
sealed interface I0 permits I1, I2 { }
sealed interface I00 permits I1, I2 { }
sealed interface I1 extends I0, I00 permits B, C { }
sealed interface I2 extends I0, I00 permits B, C { }
static final class B implements I1, I2 { }
static final class C implements I1, I2 { }
int test(Object o) {
return switch (o) {
case B c -> 2;
case C d -> 3;
};
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170079092
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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.
>
> 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 812:
> 810: if (l instanceof JCPatternCaseLabel patternLabel) {
> 811: for (Type component : components(selector.type))
> {
> 812: patterns =
> patterns.prepend(PatternDescription.from(types, component, patternLabel.pat));
I noted that this code ends up adding redundant pattern descriptions to the
list - for instance:
class Test {
sealed interface I1 permits B, C { }
sealed interface I2 permits B, C { }
static final class B implements I1, I2 { }
static final class C implements I1, I2 { }
int test(Z z) {
return switch (z) {
case B c -> 2;
case C d -> 3;
};
}
}
In this case the list ends up with 6 elements, [ B, B, B, C, C, C ]. Given that
the complexity of the algorithm depends on the number of patterns in the list,
it would probably be better to use a set here and try to make the list as small
as possible from early on.
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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170062325
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170062982
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda 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. > > 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 953: > 951: } > 952: > 953: Set 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169854290
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Mon, 17 Apr 2023 11:33:56 GMT, Maurizio Cimadamore
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 reduceBindingPatterns(Type
>> selectorType, List 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/4a7
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Fri, 14 Apr 2023 18:30:56 GMT, Vicente Romero 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/Attr.java line 4092: > >> 4090: log.error(DiagnosticFlag.SOURCE_LEVEL, tree.pos(), >> 4091: >> Feature.UNCONDITIONAL_PATTERN_IN_INSTANCEOF.error(this.sourceName)); >> 4092: allowUnconditionalPatternsInstanceOf = true; > > sorry not sure why we are doing this. Either the feature should be allowed or > not right? We normally don't produce multiple source level errors, but it is true Log does that us by itself, so no need to set the flag, fixed: https://github.com/openjdk/jdk/pull/13074/commits/bb26b52268c25863ba358843441d4c4352f877fd Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169907081
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Fri, 14 Apr 2023 16:45:36 GMT, Roger Riggs wrote: >> Well, I'm aware of this, and https://github.com/openjdk/jdk/pull/9779 even >> optimizes the case where the `enumSwitch` only gets enum constants as >> parameters. >> >> And, overall, it is fairly easy to implement, I think I've had at least one >> implementation in the past. But, the last time I was experimenting with >> this, there IIRC was a performance hit for using the indy/condy (IIRC it >> worked as a condy/ldc for a mapping array - but it could as easily be an >> indy doing the mapping as such). So, frankly, to me, simplifying the >> compiler slightly (in maybe ~10 years, because we would still need to keep >> the current desugaring for targets that don't have the bootstrap) while >> slowing down all other code is not a good balance. *If* we can make the >> indy/condy at least as fast as the current code, (or faster,) then sure, >> this is the right way to go. And, as far as javac is concerned that is not >> really difficult. > > Is it ever too early in JDK startup (Phase 1) to use #3? But you'll find out > pretty quick if the JDK won't start. But it might constrain where we can use > Pattern matching (and it won't be the first feature that can't be used in > Phase 1). FWIW, I've changed the bootstraps to not initialize the enum classes. But, I don't think I can promise `ConstantCallSite` will be used. - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169905066
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
> 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. 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. - Changes: - all: https://git.openjdk.org/jdk/pull/13074/files - new: https://git.openjdk.org/jdk/pull/13074/files/57445212..a6ba602b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13074&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13074&range=00-01 Stats: 332 lines in 8 files changed: 260 ins; 32 del; 40 mod Patch: https://git.openjdk.org/jdk/pull/13074.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13074/head:pull/13074 PR: https://git.openjdk.org/jdk/pull/13074
