RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
Evgeny Mandrikov
godin at openjdk.java.net
Tue May 25 09:32:03 UTC 2021
On Mon, 17 May 2021 19:01:01 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:
>> Good work. There's a lot to take in here. I think overall, it holds up well - I like how case labels have been extended to accommodate for patterns. In the front-end, I think there are some questions over the split between Attr and Flow - maybe it is unavoidable, but I'm not sure why some control-flow analysis happens in Attr instead of Flow and I left some questions to make sure I understand what's happening.
>>
>> In the backend it's mostly good work - but overall the structure of the code, both in Lower and in TransPattern is getting very difficult to follow, given there are many different kind of switches each with its own little twist attached to it. It would be nice, I think (maybe in a separate cleanup?) if the code paths serving the different switch kinds could be made more separate, so that, when reading the code we can worry only about one possible code shape. That would make the code easier to document as well.
>
> @mcimadamore, @forax, thanks a lot for comments! I tried to apply the requested changes in recent commits (https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99 ).
>
> I've also tried to update the implementation for the latest spec changes here:
> https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
>
> The spec changes are: total patterns are nullable; pattern matching ("new") statement switches must be complete (as switch expressions).
>
> Any further feedback is welcome!
Hi @lahodaj �� ,
I tried `javac` built from this PR and for the following `Example.java`
class Example {
void example(Object o) {
switch (o) {
case String s && s.length() == 0 ->
System.out.println("1st case");
case String s && s.length() == 1 -> // line 6
System.out.println("2nd case"); // line 7
case String s -> // line 8
System.out.println("3rd case"); // line 9
default -> // line 10
System.out.println("default case"); // line 11
}
}
}
execution of
javac --enable-preview --release 17 Example.java
javap -v -p Example.class
produces
void example(java.lang.Object);
descriptor: (Ljava/lang/Object;)V
flags: (0x0000)
Code:
stack=2, locals=7, args_size=2
0: aload_1
1: dup
2: invokestatic #7 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
5: pop
6: astore_2
7: iconst_0
8: istore_3
9: aload_2
10: iload_3
11: invokedynamic #13, 0 // InvokeDynamic #0:typeSwitch:(Ljava/lang/Object;I)I
16: tableswitch { // 0 to 2
0: 44
1: 74
2: 105
default: 122
}
44: aload_2
45: checkcast #17 // class java/lang/String
48: astore 4
50: aload 4
52: invokevirtual #19 // Method java/lang/String.length:()I
55: ifeq 63
58: iconst_1
59: istore_3
60: goto 9
63: getstatic #23 // Field java/lang/System.out:Ljava/io/PrintStream;
66: ldc #29 // String 1st case
68: invokevirtual #31 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
71: goto 133
74: aload_2
75: checkcast #17 // class java/lang/String
78: astore 5
80: aload 5
82: invokevirtual #19 // Method java/lang/String.length:()I
85: iconst_1
86: if_icmpeq 94
89: iconst_2
90: istore_3
91: goto 9
94: getstatic #23 // Field java/lang/System.out:Ljava/io/PrintStream;
97: ldc #37 // String 2nd case
99: invokevirtual #31 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
102: goto 133
105: aload_2
106: checkcast #17 // class java/lang/String
109: astore 6
111: getstatic #23 // Field java/lang/System.out:Ljava/io/PrintStream;
114: ldc #39 // String 3rd case
116: invokevirtual #31 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
119: goto 133
122: getstatic #23 // Field java/lang/System.out:Ljava/io/PrintStream;
125: ldc #41 // String default case
127: invokevirtual #31 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
130: goto 133
133: return
LineNumberTable:
line 3: 0
line 4: 44
line 5: 63
line 4: 71
line 6: 74
line 7: 94
line 6: 102
line 8: 105
line 9: 111
line 8: 119
line 11: 122
line 8: 130
line 13: 133
I believe `LineNumberTable` entries `line 6: 102`, `line 8: 119` and `line 8: 130` should not be present.
Otherwise debugger misleadingly will be showing
line `6` after step from line `7`,
line `8` after step from line `9`
and even more misleadingly line `8` after step from line `11`.
This also affects [JaCoCo](https://github.com/jacoco/jacoco) Java Code Coverage tool (I'm one of its developers), which relies on LineNumberTable to provide code coverage highlighting and these entries cause misleading highlighting - partial coverage of line `8` when default case was not executed.
Should I create separate ticket about this in https://bugs.openjdk.java.net/ or this comment here is enough?
-------------
PR: https://git.openjdk.java.net/jdk/pull/3863
More information about the core-libs-dev
mailing list