On Sat, 9 Sep 2023 00:02:20 GMT, Aggelos Biboudis <abimpou...@openjdk.org> wrote:
> This PR finalizes the feature of unnamed variables and patterns. > > --------- > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > - [ ] Change requires CSR request > [JDK-8315851](https://bugs.openjdk.org/browse/JDK-8315851) to be approved > > > > ### Reviewing > <details><summary>Using <code>git</code></summary> > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/15649/head:pull/15649` \ > `$ git checkout pull/15649` > > Update a local copy of the PR: \ > `$ git checkout pull/15649` \ > `$ git pull https://git.openjdk.org/jdk.git pull/15649/head` > > </details> > <details><summary>Using Skara CLI tools</summary> > > Checkout this PR locally: \ > `$ git pr checkout 15649` > > View PR using the GUI difftool: \ > `$ git pr show -t 15649` > > </details> > <details><summary>Using diff file</summary> > > Download this PR as a diff file: \ > <a > href="https://git.openjdk.org/jdk/pull/15649.diff";>https://git.openjdk.org/jdk/pull/15649.diff</a> > > </details> > > > ### Webrev > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/15649#issuecomment-1733906010) Overall, looks reasonable to me. A few comments/suggestions for your consideration inline. test/langtools/tools/javac/T8314423.java line 5: > 3: * @bug 8314423 > 4: * @summary Multiple patterns without unnamed variables > 5: * @compile T8314423.java I think I would keep the existing `@compile` as well, with `--release 21`, to verify the source level checks are working. test/langtools/tools/javac/diags/examples/UnderscoreAsIdentifierError.java line 26: > 24: // key: compiler.err.underscore.as.identifier > 25: // key: compiler.warn.source.no.system.modules.path > 26: // options: -source 20 Nit: either `-Xlint:-options -source 21`, or `--release 21`, and drop the `warn.source.no.system.modules.path`. test/langtools/tools/javac/diags/examples/UnderscoreInLambdaExpression.java line 24: > 22: */ > 23: > 24: // options: -Xlint:preview `-Xlint:preview` is probably unnecessary here. test/langtools/tools/javac/diags/examples/UseOfUnderscoreNotAllowedWithBrackets.java line 25: > 23: > 24: // key: compiler.err.use.of.underscore.not.allowed.with.brackets > 25: // options: -Xlint:preview The `-Xlint:preview` is probably unnecessary here. test/langtools/tools/javac/lambda/IdentifierTest.java line 7: > 5: * @summary Test generation of warnings when '_' is used an identifier > 6: * @compile/fail/ref=IdentifierTest8.out --release 8 -Werror > -XDrawDiagnostics -Xlint:-options IdentifierTest.java > 7: * @compile/fail/ref=IdentifierTest9.out -XDrawDiagnostics > IdentifierTest.java Note here we are using `ref=IdentifierTest9.out`, but there's no `--release 9` (or `--release 21`). So, it is the same as the next line. I think it would be useful if one of these lines used `--release 21` - even if the outputs are the same. test/langtools/tools/javac/lambda/IdentifierTest9.out line 33: > 31: IdentifierTest.java:152:17: compiler.err.use.of.underscore.not.allowed > 32: IdentifierTest.java:158:16: > compiler.err.use.of.underscore.not.allowed.with.brackets > 33: IdentifierTest.java:160:25: compiler.err.use.of.underscore.not.allowed Note the errors are like: /home/jlahoda/src/jdk/jdk2/test/langtools/tools/javac/lambda/IdentifierTest.java:160: error: as of release 21, the underscore keyword '_' is only allowed to declare for(String _s : _ ) { ^ unnamed patterns, local variables, exception parameters or lambda parameters At least the release version should be updated, but to not let the message to be broken into two lines like this. Maybe produce a top-level diagnostic along the line of "underscore not allowed here" with sub-diagnostic with the details (which then can (maybe?) span multiple lines). Also, the wording sounds to me like if there was a restriction in JDK 22, while underscore is actually permitted on more places than before. So, maybe something like: underscore not allowed here as of release 9, ''_'' is a keyword, and may not be used as an identifier as of release 21, ''_'' can be used as a name in the declaration of unnamed patterns, local variables, exception parameters or lambda parameters ? Just an idea. ------------- PR Review: https://git.openjdk.org/jdk/pull/15649#pullrequestreview-1642635025 PR Review Comment: https://git.openjdk.org/jdk/pull/15649#discussion_r1336168133 PR Review Comment: https://git.openjdk.org/jdk/pull/15649#discussion_r1336173607 PR Review Comment: https://git.openjdk.org/jdk/pull/15649#discussion_r1336171867 PR Review Comment: https://git.openjdk.org/jdk/pull/15649#discussion_r1336169144 PR Review Comment: https://git.openjdk.org/jdk/pull/15649#discussion_r1336171079 PR Review Comment: https://git.openjdk.org/jdk/pull/15649#discussion_r1336165928
