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? ------------- PR: https://git.openjdk.java.net/jdk/pull/3863
