----- Mail original ----- > De: "Evgeny Mandrikov" <go...@openjdk.java.net> > À: "build-dev" <build-dev@openjdk.java.net>, "compiler-dev" > <compiler-...@openjdk.java.net>, "core-libs-dev" > <core-libs-...@openjdk.java.net>, "javadoc-dev" <javadoc-...@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 <jlah...@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
