8276348: Use blessed modifier order in java.base

[pavelrappo]

This PR follows up one of the recent PRs, where I used a non-canonical modifier order. Since the problem was noticed ¹, why not to address it en masse?

As far as I remember, the first mass-canonicalization of modifiers took place in JDK-8136583 in 2015 ². That change affected 1780 lines spanning 453 files. Since then modifiers have become a bit loose, and it makes sense to re-bless (using the JDK-8136583 terminology) them.

This change was produced by running the below command followed by updating the copyright years on the affected files where necessary:

$ sh ./bin/blessed-modifier-order.sh src/java.base

The resulting change is much smaller than that of 2015: 39 lines spanning 21 files.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8276348: Use blessed modifier order in java.base

Reviewers

• Daniel Fuchs (@dfuch - Reviewer)
• Joe Darcy (@jddarcy - Reviewer)
• Iris Clark (@irisclark - Reviewer)
• Roger Riggs (@RogerRiggs - Reviewer)
• Martin Buchholz (@Martin-Buchholz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6213/head:pull/6213
$ git checkout pull/6213

Update a local copy of the PR:
$ git checkout pull/6213
$ git pull https://git.openjdk.java.net/jdk pull/6213/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6213

View PR using the GUI difftool:
$ git pr show -t 6213

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6213.diff

Footnotes

1. https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-794333365) ↩

2. http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html ↩

[bridgekeeper]

👋 Welcome back prappo! 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.

[openjdk]

@pavelrappo The following labels will be automatically applied to this pull request:

• core-libs
• i18n
• net
• nio
• security

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.

[mlbridge]

Webrevs

• 00: Full (9e463d3e)

[pavelrappo]

A colleague suggested that I should clarify that the blessed-modifier-order.sh script that I used is available in the JDK repo at https://github.com/openjdk/jdk/blob/01105d6985b39d4064b9066eab3612da5a401685/bin/blessed-modifier-order.sh. That script was contributed by Martin Buchholz in JDK-8136656 in 2015.

[dfuch]

LGTM

[openjdk]

@pavelrappo 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:

8276348: Use blessed modifier order in java.base

Reviewed-by: dfuchs, darcy, iris, rriggs, martin

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 5 new commits pushed to the master branch:

• bb92fb0: 8274930: sun/tools/jps/TestJps.java can fail with long VM arguments string
• 6a04899: 8275840: Add test to java/nio/channels/Channels/TransferTo.java to test transfer sizes > 2GB
• 01105d6: 8276367: ProblemList vmTestbase/nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption/TestDescription.java
• 8fc16f1: 8275729: Qualified method names in CodeHeap Analytics
• fa4ce82: 8276260: (se) Remove java/nio/channels/Selector/Wakeup.java from ProblemList (win)

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[prrace]

JFYI a couple of times I've wondered if we regressed on this. I just ran the script on the desktop module and we havea few instances there too, so I've filed a clean up bug on it.

[RogerRiggs]

Keep it as is with the modifiers in the recommended order. I don't think adding extra typography is warranted.

[openjdk]

Going to push as commit 6150633.
Since your change was applied there have been 9 commits pushed to the master branch:

• 465d350: 8276157: C2: Compiler stack overflow during escape analysis on Linux x86_32
• 7439b59: 8276044: ciReplay: C1 does not dump a replay file when using DumpReplay as compile command option
• 87b926e: 8275086: compiler/c2/irTests/TestPostParseCallDevirtualization.java fails when compiler1 is disabled
• 2b02b6f: 8274942: AssertionError at jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
• bb92fb0: 8274930: sun/tools/jps/TestJps.java can fail with long VM arguments string
• 6a04899: 8275840: Add test to java/nio/channels/Channels/TransferTo.java to test transfer sizes > 2GB
• 01105d6: 8276367: ProblemList vmTestbase/nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption/TestDescription.java
• 8fc16f1: 8275729: Qualified method names in CodeHeap Analytics
• fa4ce82: 8276260: (se) Remove java/nio/channels/Selector/Wakeup.java from ProblemList (win)

Your commit was automatically rebased without conflicts.

[openjdk]

@pavelrappo Pushed as commit 6150633.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from David Holmes on core-libs-dev:

On 3/11/2021 9:26 pm, Pavel Rappo wrote:

On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz <martin at openjdk.org> wrote:

Pragmatically, fix the script to ignore those keywords on comment lines. Learn Perl, its just a regular expression pattern match and replace expression.

I understand in principle how to modify that script to ignore doc comments. The thing I was referring to when said "btw, how would we do that?" was this: not all comment lines are prose. Some of those lines belong to snippets of code, which I guess you would also like to be properly formatted.

But having seen several reviewers be unmoved by the difference, the real pragmatic view is to ignore the English.

I'm sorry you feel that way. Would it be okay if I made it clear that those two words are not English adjectives but are special symbols that happen to use Latin script and originate from the English words they resemble? If so, I could enclose each of them in `{@code ... }`. If not, I could drop that particular change from this PR.

The blessed-modifier-order.sh script intentionally modifies comments, with the hope of finding code snippets (it did!)

Probably I manually deleted the change to Object.java back in 2015, to avoid the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.

I agree with @pavelrappo that script-generated changes should not be mixed with manual changes.
I would also not update copyright years for such changes.

It's a feature of blessed-modifier-order.sh that all existing formatting is perfectly preserved.

One more thing. Please have a look at this other line in the same file; this line was there before the change https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49

So before the change, the file was somewhat inconsistent. The change made it consistent. **If one is going to ever revert that controversial part of the change, please update both lines so that the file remains consistent.**

Line 281 is (was!) consistent with line 277 because it is distinguishing
a synchronized "static method" from a synchronized "instance method".
There was no need to make any change to line 281 because of the
blessed-order of modifiers as defined for method declarations! This text
is just prose. Now for consistency you should change line 277 to refer
to a "non-static synchronized method" (as "instance synchronized method"
would be truly awful).

Line 49 places "static synchronized" in code font, implying that it is
referring to the actual method modifiers and as such using the blessed
order is appropriate. Line 49 does not need to be "consistent" with line
281. If line 49 used a normal font so the words "static" and
"synchronized" were just prose then either order would be perfectly fine
in terms of English language, but then you could make a case for having
it be consistent with line 281.

Cheers,
David
-----

[pavelrappo]

Mailing list message from David Holmes on core-libs-dev:

On 3/11/2021 9:26 pm, Pavel Rappo wrote:

On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz wrote:

Pragmatically, fix the script to ignore those keywords on comment lines. Learn Perl, its just a regular expression pattern match and replace expression.

I understand in principle how to modify that script to ignore doc comments. The thing I was referring to when said "btw, how would we do that?" was this: not all comment lines are prose. Some of those lines belong to snippets of code, which I guess you would also like to be properly formatted.

But having seen several reviewers be unmoved by the difference, the real pragmatic view is to ignore the English.

I'm sorry you feel that way. Would it be okay if I made it clear that those two words are not English adjectives but are special symbols that happen to use Latin script and originate from the English words they resemble? If so, I could enclose each of them in {@code ... }. If not, I could drop that particular change from this PR.

The blessed-modifier-order.sh script intentionally modifies comments, with the hope of finding code snippets (it did!)
Probably I manually deleted the change to Object.java back in 2015, to avoid the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.
I agree with @pavelrappo that script-generated changes should not be mixed with manual changes.
I would also not update copyright years for such changes.
It's a feature of blessed-modifier-order.sh that all existing formatting is perfectly preserved.

One more thing. Please have a look at this other line in the same file; this line was there before the change

jdk/src/java.base/share/classes/java/lang/Object.java

Line 49 in 465d350

* static synchronized} methods of the represented class.

So before the change, the file was somewhat inconsistent. The change made it consistent. If one is going to ever revert that controversial part of the change, please update both lines so that the file remains consistent.

Line 281 is (was!) consistent with line 277 because it is distinguishing a synchronized "static method" from a synchronized "instance method". There was no need to make any change to line 281 because of the blessed-order of modifiers as defined for method declarations! This text is just prose. Now for consistency you should change line 277 to refer to a "non-static synchronized method" (as "instance synchronized method" would be truly awful).

Thanks, David. You've provided a clear and convincing argument, and I can see the inconsistency I introduced. I can revert that particular piece back if you think that it would be appropriate.

That said, this line will have to be filtered out every time the script is run. I could probably provide a more involved script that harnesses the power of AST (com.sun.source.doctree) to try to filter out prose, but it would be impossible to beat the simplicity of the current script; and simplicity is also important.

Line 49 places "static synchronized" in code font, implying that it is referring to the actual method modifiers and as such using the blessed order is appropriate. Line 49 does not need to be "consistent" with line 281. If line 49 used a normal font so the words "static" and "synchronized" were just prose then either order would be perfectly fine in terms of English language, but then you could make a case for having it be consistent with line 281.

I've been always having hard time with modifiers being not enclosed in @code in the first place; they are barely English words. Is there really a semantic difference between L49 and L281 such that it warrants the use of @code in the former but not in the latter case? Does synchornized or static in L281 refer to anything other than the like-named Java modifiers?

[mlbridge]

Mailing list message from David Holmes on core-libs-dev:

On 4/11/2021 12:14 am, Pavel Rappo wrote:

On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

This PR follows up one of the recent PRs, where I used a non-canonical modifier order. Since the problem was noticed [^1], why not to address it en masse?

As far as I remember, the first mass-canonicalization of modifiers took place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 files. Since then modifiers have become a bit loose, and it makes sense to re-bless (using the JDK-8136583 terminology) them.

This change was produced by running the below command followed by updating the copyright years on the affected files where necessary:

 \$ sh \.\/bin\/blessed\-modifier\-order\.sh src\/java\.base

The resulting change is much smaller than that of 2015: 39 lines spanning 21 files.

[^1]: https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html (or https://github.com//pull/6191#pullrequestreview-794333365)
[^2]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [core-libs-dev](mailto:core-libs-dev at mail.openjdk.java.net):_

On 3/11/2021 9:26 pm, Pavel Rappo wrote:

On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz <martin at openjdk.org> wrote:

Pragmatically, fix the script to ignore those keywords on comment lines. Learn Perl, its just a regular expression pattern match and replace expression.

I understand in principle how to modify that script to ignore doc comments. The thing I was referring to when said "btw, how would we do that?" was this: not all comment lines are prose. Some of those lines belong to snippets of code, which I guess you would also like to be properly formatted.

But having seen several reviewers be unmoved by the difference, the real pragmatic view is to ignore the English.

I'm sorry you feel that way. Would it be okay if I made it clear that those two words are not English adjectives but are special symbols that happen to use Latin script and originate from the English words they resemble? If so, I could enclose each of them in `{@code ... }`. If not, I could drop that particular change from this PR.

The blessed-modifier-order.sh script intentionally modifies comments, with the hope of finding code snippets (it did!)
Probably I manually deleted the change to Object.java back in 2015, to avoid the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.
I agree with @pavelrappo that script-generated changes should not be mixed with manual changes.
I would also not update copyright years for such changes.
It's a feature of blessed-modifier-order.sh that all existing formatting is perfectly preserved.

One more thing. Please have a look at this other line in the same file; this line was there before the change https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it consistent. **If one is going to ever revert that controversial part of the change, please update both lines so that the file remains consistent.**

Line 281 is (was!) consistent with line 277 because it is distinguishing a synchronized "static method" from a synchronized "instance method". There was no need to make any change to line 281 because of the blessed-order of modifiers as defined for method declarations! This text is just prose. Now for consistency you should change line 277 to refer to a "non-static synchronized method" (as "instance synchronized method" would be truly awful).

Thanks, David. You've provided a clear and convincing argument, and I can see the inconsistency I introduced. I can revert that particular piece back if you think that it would be appropriate.

That said, this line will have to be filtered out every time the script is run. I could probably provide a more involved script that harnesses the power of AST (com.sun.source.doctree) to try to filter out prose, but it would be impossible to beat the simplicity of the current script; and simplicity is also important.

Given this is prose, the adjectives should be separated by commas: "a
synchronized, static method", and "a synchronized, instance method".
Does that avoid the problem with the script?

Line 49 places "static synchronized" in code font, implying that it is referring to the actual method modifiers and as such using the blessed order is appropriate. Line 49 does not need to be "consistent" with line 281. If line 49 used a normal font so the words "static" and "synchronized" were just prose then either order would be perfectly fine in terms of English language, but then you could make a case for having it be consistent with line 281.

I've been always having hard time with modifiers being not enclosed in `@code` in the first place; they are barely English words. Is there really a semantic difference between L49 and L281 such that it warrants the use of `@code` in the former but not in the latter case? Does `synchornized` or `static` in L281 refer to anything other than the like-named Java modifiers?

Consider this definition:

"A synchronized method is one which must acquire the monitor of the
Object upon which the method is invoked, and is indicated by applying
the {@code synchronized} modifier to the method declaration."

Here there is a distinction** between the general notion of a
"synchronized method" and the "synchronized" modifier. Obviously they
are strongly related, and often could be used interchangeably, but you
can also find places where it is more appropriate to use one over the
other. So yes it is hard, but context can influence the choice: is this
text referring to the general concept of a synchronized/static method,
or to the use of the modifier? Line 49 could have gone either way IMO.

** The distinction would be more obvious if Java had an implicit way to
define synchronized methods. So think about the concept of "package
private" access - there is no package-private modifier so you
wouldn't/shouldn't ever write "package private" in code font.

Cheers,
David

P.S. For the book "The Java Programming Language" the authors made a
very conscious decision to not put the word "synchronized" in code font
every time it was used in the text, but reserved the code font for
specific usages. The same applies to other modifiers: static, public,
etc. Other authors have made similar decisions.