Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 22:31:15 GMT, Jonathan Gibbons wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > incorporate review comments Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19130#pullrequestreview-2091633348
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 22:31:15 GMT, Jonathan Gibbons wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > incorporate review comments Thanks for making these updates. To my eye, replacement of "///" with "//---" is preferable over "//|", but I think either choice is acceptable. - Marked as reviewed by iris (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19130#pullrequestreview-2091379761
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 22:31:15 GMT, Jonathan Gibbons wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > incorporate review comments I like `//---` ; +1! - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2137452920
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 20:26:40 GMT, Naoto Sato wrote: >> As a non-standard comment, it will trigger a warning (and hence an error), >> since the prevailing standard for `java.base` is to compile with all >> warnings enabled (`-Xlint`) and no warnings found (verified by `-Werror`) >> >> The alternative would be to use `@SuppressWarnings` for the >> `dangling-doc-comment` warning, but that too would be a code change to these >> imported files. > > OK, we will need to live with it. Yes, while there is a strong preference to leave upstream sources unedited in the JDK, to my mind being able to compile the java.base module with all warnings enabled should take precedence. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1618046467
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
> With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: incorporate review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19130/files - new: https://git.openjdk.org/jdk/pull/19130/files/3e039b43..e77a4d14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19130=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19130=00-01 Stats: 24 lines in 7 files changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/19130.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130 PR: https://git.openjdk.org/jdk/pull/19130
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 20:22:24 GMT, Jonathan Gibbons wrote: > What about changing `///` to `//---` to give slightly more prominence to > these comments, over plain old `//` comments. The dashes give a small sense > of a horizontal rule, to delimit sections of code. > > (FWIW, I have locally edited `//|` to `//` and such comments do not stand out > beside existing use of `//`.) `//---` seems okay, it would stand out a bit more. - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136060411
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 18:50:38 GMT, Jonathan Gibbons wrote: >> src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java >> line 122: >> >>> 120: * see store.c of gennorm for more information and values >>> 121: */ >>> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */ >> >> This source file is coming from the upstream ICU4J project. Even if this is >> a `non-standard` comment, I would keep it as it is to minimize the merge >> effort. > > As a non-standard comment, it will trigger a warning (and hence an error), > since the prevailing standard for `java.base` is to compile with all warnings > enabled (`-Xlint`) and no warnings found (verified by `-Werror`) > > The alternative would be to use `@SuppressWarnings` for the > `dangling-doc-comment` warning, but that too would be a code change to these > imported files. OK, we will need to live with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1617854422
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). `GregorianCalendar` and ICU4J files LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19130#pullrequestreview-2083793439
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 20:01:46 GMT, Alan Bateman wrote: > > OK. I was just trying to honor the apparent intent to make the comment > > stand out more than just a plain `//` comment, but I have no strong > > feelings against reducing `///` to `//` > > In this case I would reduce it to '//' but others will have different > opinions of course. What about changing `///` to `//---` to give slightly more prominence to these comments, over plain old `//` comments. The dashes give a small sense of a horizontal rule, to delimit sections of code. (FWIW, I have locally edited `//|` to `//` and such comments do not stand out beside existing use of `//`.) - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136042843
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 18:57:07 GMT, Jonathan Gibbons wrote: > OK. I was just trying to honor the apparent intent to make the comment stand > out more than just a plain `//` comment, but I have no strong feelings > against reducing `///` to `//` In this case I would reduce it to '//' but others will have different opinions of course. - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136012355
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 23 May 2024 05:52:57 GMT, Alan Bateman wrote: > > A long vertical series of lines beginning /// is replaced by lines > > beginning //|. > > This one looks unusual when it's just one line, I could imagine deleting the > "|" in these cases. OK. I was just trying to honor the apparent intent to make the comment stand out more than just a plain `//` comment, but I have no strong feelings against reducing `///` to `//` - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2135914838
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Wed, 22 May 2024 20:13:08 GMT, Naoto Sato wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java > line 122: > >> 120: * see store.c of gennorm for more information and values >> 121: */ >> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */ > > This source file is coming from the upstream ICU4J project. Even if this is a > `non-standard` comment, I would keep it as it is to minimize the merge effort. As a non-standard comment, it will trigger a warning, since the prevailing standard for `java.base` is to compile with all warnings enabled and no warnings found. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1617755455
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > A long vertical series of lines beginning /// is replaced by lines beginning > //|. This one looks unusual when it's just one line, I could imagine deleting the "|" in these cases. - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2126289283
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote:
> With the advent of JEP 467, `///` comments may be treated as documentation
> comments, and may be subject to the recently new `javac` warning about
> "dangling doc comments" in unexpected places.
>
> In keeping with the policy to keep the `java.base` module free of all `javac`
> warnings, this patch proposes edits to existing uses of `///`.
>
> There are two dominant policies in the proposed changes.
> 1. A long horizontal line of `/` is replaced by `//-`
> 2. A long vertical series of lines beginning `///` is replaced by lines
> beginning `//|`.
>
> As with all style changes, I have also tried to honor local usage, for
> consistency.
>
> In one place, a pair of comments appeared to contain directives (`CLOVER:ON`,
> `CLOVER:OFF`). I investigated the use of such comments to determine that the
> exact form of the comment prefix was not significant. (Phew!)
>
>
> (This PR is informally blocked by JEP 467).
src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java
line 122:
> 120: * see store.c of gennorm for more information and values
> 121: */
> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */
This source file is coming from the upstream ICU4J project. Even if this is a
`non-standard` comment, I would keep it as it is to minimize the merge effort.
src/java.base/share/classes/jdk/internal/icu/lang/UCharacterDirection.java line
61:
> 59: {
> 60: }
> 61: // CLOVER:ON
Same here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1610588637
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1610586405
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). Will we incorporate the changes for JDK-8 target code as in #19151? - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2118330326
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 206: > 204: //- > 205: // Constants // > 206: //- Suggestion: //---// // Constants // //---// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 227: > 225: //--- > 226: // Local variable loads and stores // > 227: //--- Suggestion: //-// // Local variable loads and stores // //-// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 411: > 409: //- > 410: // Widening conversions only // > 411: //- Suggestion: //---// // Widening conversions only // //---// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 439: > 437: // > 438: // Control flow // > 439: // Suggestion: //--// // Control flow // //--// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 519: > 517: //--- > 518: // Return instructions // > 519: //--- Suggestion: //-// // Return instructions // //-// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 553: > 551: // > 552: // Field operations // > 553: // Suggestion: //--// // Field operations // //--// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 581: > 579: //-- > 580: // Method invocations // > 581: //-- Suggestion: //// // Method invocations // //// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 632: > 630: // > 631: // Array length // > 632: // Suggestion: //--// // Array length // //--// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 640: > 638: //--- > 639: // New // > 640: //--- Suggestion: //-// // New // //-// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 650: > 648: //-- > 649: // Athrow // > 650: //-- Suggestion: //// // Athrow // //// src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 659: > 657: // > 658: // Checkcast and instanceof // > 659: // Suggestion: //--// // Checkcast and instanceof // //--// - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593971885 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593972168 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593972556 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593972931 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593973196 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593973651 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593974149 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593974540 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593974850 PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1593975284 PR Review Comment:
RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
With the advent of JEP 467, `///` comments may be treated as documentation comments, and may be subject to the recently new `javac` warning about "dangling doc comments" in unexpected places. In keeping with the policy to keep the `java.base` module free of all `javac` warnings, this patch proposes edits to existing uses of `///`. There are two dominant policies in the proposed changes. 1. A long horizontal line of `/` is replaced by `//-` 2. A long vertical series of lines beginning `///` is replaced by lines beginning `//|`. As with all style changes, I have also tried to honor local usage, for consistency. In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to determine that the exact form of the comment prefix was not significant. (Phew!) (This PR is informally blocked by JEP 467). - Commit messages: - incorporate review comments - Merge remote-tracking branch 'upstream/master' into 8330177.dangling-java.base - JDK-8331879: Clean up non-standard use of /// comments in `java.base` Changes: https://git.openjdk.org/jdk/pull/19130/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19130=00 Issue: https://bugs.openjdk.org/browse/JDK-8331879 Stats: 117 lines in 25 files changed: 0 ins; 0 del; 117 mod Patch: https://git.openjdk.org/jdk/pull/19130.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130 PR: https://git.openjdk.org/jdk/pull/19130
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Fri, 10 May 2024 01:25:45 GMT, xiaotaonan wrote: >> Clean up non-standard use of /// comments in `java.base` > > @mdinacci @hns @landonf Hello @xiaotaonan, like Jon noted in his comment, there's already another PR addressing this change. So I think this current PR can be closed. - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2113730374
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Fri, 10 May 2024 01:25:45 GMT, xiaotaonan wrote: >> Clean up non-standard use of /// comments in `java.base` > > @mdinacci @hns @landonf Hello @xiaotaonan, I see that you have several PRs of similar nature that have been opened in the past day or two. I would recommend taking a look at the OpenJDK developer's guide https://openjdk.org/guide/ (if you haven't already) to get familiar with the contribution guidelines. Additionally, if an issue has been assigned to someone, it's recommended that you ask the person if you can take over the issue before starting any work on it. Finally, although not applicable to this specific PR, some of the other PRs you have open involve code changes that require jtreg test cases to be included and also making sure existing tests are run and continue to pass. The testing guidelines are available in the same guide https://openjdk.org/guide/#testing-the-jdk. - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2105551862
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 9 May 2024 02:09:50 GMT, xiaotaonan wrote: > Clean up non-standard use of /// comments in `java.base` This PR is premature. Until JEP 467 is integrated, there is nothing special about `///` comments, and the compiler does not report on non-standard use. There is a Draft PR for this issue ready to go once JEP 467 has been integrated, that addresses all necessary comments in `java.base`. https://github.com/openjdk/jdk/pull/19130 - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2104836545
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 9 May 2024 02:09:50 GMT, xiaotaonan wrote: > Clean up non-standard use of /// comments in `java.base` @mdinacci @hns @landonf - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2103685769
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 9 May 2024 02:09:50 GMT, xiaotaonan wrote: > Clean up non-standard use of /// comments in `java.base` Related to #19130. Good catch, these were probably not detected because they were compiled at Java 8 language level and thus not detected by the new compiler warnings. - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2101834976
RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
Clean up non-standard use of /// comments in `java.base` - Commit messages: - Clean up non-standard use of /// comments in Changes: https://git.openjdk.org/jdk/pull/19151/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19151=00 Issue: https://bugs.openjdk.org/browse/JDK-8331879 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19151.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19151/head:pull/19151 PR: https://git.openjdk.org/jdk/pull/19151
