jdk
jdk copied to clipboard
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.
- A long horizontal line of
/////is replaced by//----- - 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).
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8331879: Clean up non-standard use of /// comments in
java.base(Sub-task - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130
$ git checkout pull/19130
Update a local copy of the PR:
$ git checkout pull/19130
$ git pull https://git.openjdk.org/jdk.git pull/19130/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19130
View PR using the GUI difftool:
$ git pr show -t 19130
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19130.diff
Webrev
:wave: Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
@jonathan-gibbons This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8331879: Clean up non-standard use of /// comments in `java.base`
Reviewed-by: naoto, iris, darcy
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 188 new commits pushed to the master branch:
- 8aeada105acd143b38b02123377ef86513eee266: 8331159: VM build without C2 fails after JDK-8180450
- e99f6a65a8307e6b31a08a677914dfd20d46687f: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
- e650bdf4654a0459bb2af95f08ba42ca870642d4: 8332507: compilation result depends on compilation order
- e4fbb15c6a7b18f1ec66176080404818d3871194: 8320215: HeapDumper can use DumpWriter buffer during merge
- 681137cad2b1de8a0af1dfea949439bcaf5e7500: 8333131: Source launcher should work with service loader SPI
- 914423e3b7162ad934fa4edc46ee37e0f401d27b: 8332899: RISC-V: add comment and make the code more readable (if possible) in MacroAssembler::movptr
- 5abc02927b480a85fadecf8d03850604510276e4: 8331877: JFR: Remove JIInliner framework
- d9e7b7e7da98a0170d26301a4bbd61aad0127c6e: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
- 1e04ee6d57d5fe84e1d202b16e8d13dc13c002ff: 8331579: Reference to primitive type fails without error or warning
- 32ee252c455d3ddcb5954698b546ac39a40515e8: 8333169: javac NullPointerException record.type
- ... and 178 more: https://git.openjdk.org/jdk/compare/0a58cffe88ba823e71fcdcca64b784ed04ca5398...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
@jonathan-gibbons The following labels will be automatically applied to this pull request:
core-libsi18n
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.
Will we incorporate the changes for JDK-8 target code as in #19151?
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.
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 //
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.
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 //.)
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.
I like //--- ; +1!
/integrate
Going to push as commit 10eb1cb639095caa2636cc87c45201d4f8cf1eb4.
Since your change was applied there have been 190 commits pushed to the master branch:
- 2cae9a0397f4e46c6faec0a998ecad1c7015564d: 8314480: Memory ordering spec updates in java.lang.ref
- 9fd0e7349ebf4a49b5c0c7a16c866b5b8e626b53: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option
- 8aeada105acd143b38b02123377ef86513eee266: 8331159: VM build without C2 fails after JDK-8180450
- e99f6a65a8307e6b31a08a677914dfd20d46687f: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
- e650bdf4654a0459bb2af95f08ba42ca870642d4: 8332507: compilation result depends on compilation order
- e4fbb15c6a7b18f1ec66176080404818d3871194: 8320215: HeapDumper can use DumpWriter buffer during merge
- 681137cad2b1de8a0af1dfea949439bcaf5e7500: 8333131: Source launcher should work with service loader SPI
- 914423e3b7162ad934fa4edc46ee37e0f401d27b: 8332899: RISC-V: add comment and make the code more readable (if possible) in MacroAssembler::movptr
- 5abc02927b480a85fadecf8d03850604510276e4: 8331877: JFR: Remove JIInliner framework
- d9e7b7e7da98a0170d26301a4bbd61aad0127c6e: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
- ... and 180 more: https://git.openjdk.org/jdk/compare/0a58cffe88ba823e71fcdcca64b784ed04ca5398...master
Your commit was automatically rebased without conflicts.
@jonathan-gibbons Pushed as commit 10eb1cb639095caa2636cc87c45201d4f8cf1eb4.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.