jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8331879: Clean up non-standard use of /// comments in `java.base`

Open jonathan-gibbons opened this issue 1 year ago • 5 comments

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).


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

Link to Webrev Comment

jonathan-gibbons avatar May 07 '24 22:05 jonathan-gibbons

: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.

bridgekeeper[bot] avatar May 07 '24 22:05 bridgekeeper[bot]

@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.

openjdk[bot] avatar May 07 '24 22:05 openjdk[bot]

@jonathan-gibbons The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

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.

openjdk[bot] avatar May 07 '24 22:05 openjdk[bot]

Webrevs

mlbridge[bot] avatar May 17 '24 20:05 mlbridge[bot]

Will we incorporate the changes for JDK-8 target code as in #19151?

liach avatar May 17 '24 20:05 liach

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.

AlanBateman avatar May 23 '24 05:05 AlanBateman

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 //

jonathan-gibbons avatar May 28 '24 18:05 jonathan-gibbons

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.

AlanBateman avatar May 28 '24 20:05 AlanBateman

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 //.)

jonathan-gibbons avatar May 28 '24 20:05 jonathan-gibbons

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.

AlanBateman avatar May 28 '24 20:05 AlanBateman

I like //--- ; +1!

dfuch avatar May 29 '24 13:05 dfuch

/integrate

jonathan-gibbons avatar May 31 '24 22:05 jonathan-gibbons

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.

openjdk[bot] avatar May 31 '24 22:05 openjdk[bot]

@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.

openjdk[bot] avatar May 31 '24 22:05 openjdk[bot]