<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office"><head><!--[if gte mso 9]><xml><o:OfficeDocumentSettings><o:AllowPNG/><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml><![endif]--></head><body>
Thanks, I'll take a look.<br><br><br>SP LLC<br><p class="yahoo-quoted-begin" style="font-size: 15px; color: #715FFA; padding-top: 15px; margin-top: 0">On Monday, April 17, 2023, 8:46 PM, Archie L. Cobbs <duke@openjdk.org> wrote:</p><blockquote class="iosymail"><div dir="ltr">On Mon, 17 Apr 2023 23:43:20 GMT, Jonathan Gibbons <<a shape="rect" ymailto="mailto:jjg@openjdk.org" href="mailto:jjg@openjdk.org">jjg@openjdk.org</a>> wrote:<br clear="none"><br clear="none">>> Archie L. Cobbs has updated the pull request incrementally with one additional commit since the last revision:<br clear="none">>> <br clear="none">>> Use ToolBox and add more tests per review suggestion.<br clear="none">><br clear="none">> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 3860:<br clear="none">> <br clear="none">>> 3858: break;<br clear="none">>> 3859: nextToken();<br clear="none">>> 3860: }<br clear="none">> <br clear="none">> This might work, but it "feels" wrong (as in, a hack) to leave an extra semicolon around.<br clear="none">> <br clear="none">> Yes, a package statement must be followed by a semicolon (line 3835) but after that can't you slurp up all the extra semicolons, and only report them if there is an import statement, and ignore them if there are no imports?<br clear="none"><br clear="none">I'd rather not mess with it because the existing code is already a customized (i.e., not a straightforward parse) so that it can provide a more helpful error message when duplicate semicolons are found after import statements. See [JDK-8027682](https://bugs.openjdk.org/browse/JDK-8027682) and PR #12448. Doing what you suggest would probably be more straightforward but would risk breaking that.<br clear="none"><br clear="none">> test/langtools/tools/javac/parser/ExtraPackageSemicolon.java line 42:<br clear="none">> <br clear="none">>> 40: <br clear="none">>> 41: public static void runTest(String filename, String source) throws Exception {<br clear="none">>> 42: final File sourceFile = new File(filename);<br clear="none">> <br clear="none">> As a matter of style, writing source code to `java.io.File` objects, this feels very old-school, but for a small test program like this, it's not awful.<br clear="none">> <br clear="none">> If this were any bigger, I'd suggest looking at the test `ToolBox` library and/or using `SimpleJavaFileObject` to use more common code idioms and support.<br clear="none">> <br clear="none">> As it is, I'd still suggest using `Files.writeString` to write each source.<br clear="none">> https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/nio/file/Files.html#writeString(java.nio.file.Path,java.lang.CharSequence,java.nio.charset.Charset,java.nio.file.OpenOption...)<br clear="none"><br clear="none">I'll switch to using `ToolBox`.<br clear="none"><br clear="none">> test/langtools/tools/javac/parser/ExtraPackageSemicolon.java line 45:<br clear="none">> <br clear="none">>> 43: System.err.println("writing: " + sourceFile);<br clear="none">>> 44: try (PrintStream output = new PrintStream(new FileOutputStream(sourceFile))) {<br clear="none">>> 45: output.println(source);<br clear="none">> <br clear="none">> This would be somewhat overly simplistic (but not wrong) if any source had newlines in it.<br clear="none"><br clear="none">Fixed by using `ToolBox`.<br clear="none"><br clear="none">> test/langtools/tools/javac/parser/ExtraPackageSemicolon.java line 65:<br clear="none">> <br clear="none">>> 63: runTest("Test2.java", "package p;;");<br clear="none">>> 64: runTest("Test3.java", "package p;; ;; ;; ;;; ;;; ;;; ;;");<br clear="none">>> 65: }<br clear="none">> <br clear="none">> I suggest including test cases where the package statement and semicolons are followed by something else, like an import declaration or a type declaration.<br clear="none"><br clear="none">I'll add more tests, but extra semicolons prior to import statements aren't allowed; see [JDK-8027682].<br clear="none"><br clear="none">-------------<br clear="none"><br clear="none">PR Review Comment: <a shape="rect" href="https://git.openjdk.org/jdk/pull/13361#discussion_r1169404766" target="_blank">https://git.openjdk.org/jdk/pull/13361#discussion_r1169404766</a><br clear="none">PR Review Comment: <a shape="rect" href="https://git.openjdk.org/jdk/pull/13361#discussion_r1169404540" target="_blank">https://git.openjdk.org/jdk/pull/13361#discussion_r1169404540</a><br clear="none">PR Review Comment: <a shape="rect" href="https://git.openjdk.org/jdk/pull/13361#discussion_r1169405308" target="_blank">https://git.openjdk.org/jdk/pull/13361#discussion_r1169405308</a><div class="yqt0584384443" id="yqtfd81009"><br clear="none">PR Review Comment: </div><a shape="rect" href="https://git.openjdk.org/jdk/pull/13361#discussion_r1169404706" target="_blank">https://git.openjdk.org/jdk/pull/13361#discussion_r1169404706</a><div class="yqt0584384443" id="yqtfd56511"><br clear="none"></div></div><blockquote></blockquote></blockquote>
</body></html>