RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
Remi Forax
forax at univ-mlv.fr
Tue May 25 11:42:52 UTC 2021
----- Mail original -----
> De: "Evgeny Mandrikov" <godin at openjdk.java.net>
> À: "build-dev" <build-dev at openjdk.java.net>, "compiler-dev" <compiler-dev at openjdk.java.net>, "core-libs-dev"
> <core-libs-dev at openjdk.java.net>, "javadoc-dev" <javadoc-dev at openjdk.java.net>
> Envoyé: Mardi 25 Mai 2021 11:32:03
> Objet: Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
> 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?
Also why invokedynamic take a constant int as second argument ?
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3863
Rémi
More information about the build-dev
mailing list