8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed #17242

pavelrappo · 2024-01-03T12:09:42Z

Please review this PR to update blessed-modifier.order.sh for the default, sealed, non-sealed, and strictfp modifiers.

In a discussion preceding this PR, it was agreed that the script should better refer to relevant JLS sections rather than to java.lang.reflect.Modifier#toString, which does not model Java source.

the sealed and non-sealed class or interface modifiers were introduced in JEP 409, but have never been accounted for
the default method modifier was introduced in Java 8, but has not been explicitly accounted for. It's unclear whether it was an omission or a conscious decision. The current edition of JLS suggests that in compilable, modern and customary formatted code, default appears alone and, thus, is always canonically ordered.

However, a somewhat similar argument applies to the public modifier on an interface method. Its use is discouraged, yet the script would enforce its order if public appears not alone.

So for completeness, this PR proposes to explicitly account for default.

While not proposing to do anyting about it, this PR recognises (for the record) that strictfp class, interface, or method modifier became obsolete in JDK 17.

I've run the updated script on JDK. The script found 8 missorted sealed and 20 missorted default. The script also found 3 false positive default:

jdk/src/java.desktop/share/classes/java/awt/dnd/DragSource.java

Lines 526 to 527 in 249d553

* that implements the default

* abstract subclass of {@code DragGestureRecognizer}
jdk/src/java.management/share/classes/javax/management/MBeanServerFactory.java

Line 91 in 98da03a

* default public constructor.
jdk/src/java.desktop/share/classes/javax/print/PrintServiceLookup.java

Line 193 in 6765f90

* there is no clearly identifiable platform native default print service,

None of those findings are addressed in this PR.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed (Bug - P4)

Reviewers

Erik Joelsson (@erikj79 - Reviewer)
Roger Riggs (@RogerRiggs - Reviewer)
Martin Buchholz (@Martin-Buchholz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17242/head:pull/17242
$ git checkout pull/17242

Update a local copy of the PR:
$ git checkout pull/17242
$ git pull https://git.openjdk.org/jdk.git pull/17242/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17242

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17242.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-03T12:11:18Z

👋 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 · 2024-01-03T12:11:59Z

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

build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-01-03T12:15:04Z

Webrevs

00: Full (ce8e6668)

openjdk · 2024-01-03T13:50:49Z

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

8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed

Reviewed-by: erikj, 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 14 new commits pushed to the master branch:

4db7a1c: 8322818: Thread::getStackTrace can fail with InternalError if virtual thread is timed-parked when pinned
755722c: 8322214: Return value of XMLInputFactory.getProperty() changed from boolean to String in JDK 22 early access builds
1cf9335: 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC
13c1148: 8321599: Data loss in AVX3 Base64 decoding
028ec7e: 8319948: jcmd man page needs to be updated
54b3cee: 8322877: java/io/BufferedInputStream/TransferTo.java failed with IndexOutOfBoundsException
30a0c61: 8322879: Eliminate -Wparentheses warnings in x86-32 code
409a39e: 8320276: Improve class initialization barrier in TemplateTable::_new
296c5b6: 8322873: Duplicate -ljava -ljvm options for libinstrument
cc9ab5f: 8322963: ProblemList java/io/BufferedInputStream/TransferTo.java
... and 4 more: https://git.openjdk.org/jdk/compare/a8e4229852fac703c6271aa8c5f94f67bea44902...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.

Martin-Buchholz · 2024-01-03T17:16:37Z

bin/blessed-modifier-order.sh

    echo "Tries to get it right even within javadoc comments,"
    echo "and even if the list of modifiers spans 2 lines."
    echo
    echo "See:"
-    echo "https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/Modifier.html#toString-int-"


Looking at the latest docs for Modifier#toString()
it also needs a corresponding update
https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int)

and if that is done, then we can keep referring to its javadoc.

References to javadoc and jls both have problems of becoming stale over time.
There is no way to reference the "latest" spec via a URL.

it also needs a corresponding update
https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int)

From the preceding discussion, it follows that it cannot be done: https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117381.html

Thanks ... now I see ...

When I originally wrote this script I recall hitting some false positives - a price we pay for having this modify comments. Don't blindly apply all suggestions!

I don't recall considering "default".

It seems we no longer have a single canonical reference for modifier order.
(or more generally, a well-maintained high quality Java program style guide ...)

Thanks ... now I see ...

FWIW, @jddarcy thinks we can still improve Modifier.toString(int). See this comment and its parent thread: #17239 (comment)

pavelrappo · 2024-01-09T11:35:29Z

/integrate

openjdk · 2024-01-09T11:36:38Z

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

7d42aa1: 8310277: jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java fails with IllegalStateException
6e9671a: 8323264: Serial: Remove unused GenerationBlockSizeClosure
52a6c37: 8322858: compiler/c2/aarch64/TestFarJump.java fails on AArch64 due to unexpected PrintAssembly output
075fed9: 8323241: jcmd manpage should use lists for argument lists
30f93a2: 8320069: RISC-V: Add Zcb instructions
4cf131a: 8319716: RISC-V: Add SHA-2
7286f52: 8322829: Refactor nioBlocker to avoid blocking while holding Thread's interrupt lock
07fce8e: 8320864: Serial: Extract out Full GC related fields from ContiguousSpace
176606d: 8310995: missing @SInCE tags in 36 jdk.dynalink classes
8ae309e: 8318971: Better Error Handling for Jar Tool When Processing Non-existent Files
... and 71 more: https://git.openjdk.org/jdk/compare/a8e4229852fac703c6271aa8c5f94f67bea44902...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-09T11:36:44Z

@pavelrappo Pushed as commit 37a6172.

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

magicus · 2024-01-11T12:29:01Z

@pavelrappo

I've run the updated script on JDK. The script found 8 missorted sealed and 20 missorted default. The script also found 3 false positive default:[...] None of those findings are addressed in this PR.

Are you planning on addressing this in a separate PR? If not, maybe you can at least open an issue in JBS.

pavelrappo · 2024-01-11T14:46:16Z

@pavelrappo

I've run the updated script on JDK. The script found 8 missorted sealed and 20 missorted default. The script also found 3 false positive default:[...] None of those findings are addressed in this PR.

Are you planning on addressing this in a separate PR? If not, maybe you can at least open an issue in JBS.

I was not planning to do that, but I sure can. I'll try to publish the PR within a week. Thanks.

Initial commit

ce8e666

openjdk bot added the rfr Pull request is ready for review label Jan 3, 2024

openjdk bot added the build build-dev@openjdk.org label Jan 3, 2024

erikj79 approved these changes Jan 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 3, 2024

RogerRiggs approved these changes Jan 3, 2024

View reviewed changes

Martin-Buchholz reviewed Jan 3, 2024

View reviewed changes

Martin-Buchholz approved these changes Jan 4, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 9, 2024

openjdk bot closed this Jan 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 9, 2024

pavelrappo mentioned this pull request Jan 17, 2024

8324053: Use the blessed modifier order for sealed in java.base #17468

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed #17242

8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed #17242

pavelrappo commented Jan 3, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 3, 2024

openjdk bot commented Jan 3, 2024

mlbridge bot commented Jan 3, 2024

openjdk bot commented Jan 3, 2024 •

edited

Loading

Martin-Buchholz Jan 3, 2024

pavelrappo Jan 3, 2024

Martin-Buchholz Jan 4, 2024

pavelrappo Jan 4, 2024

pavelrappo commented Jan 9, 2024

openjdk bot commented Jan 9, 2024

openjdk bot commented Jan 9, 2024

magicus commented Jan 11, 2024

pavelrappo commented Jan 11, 2024

	* that implements the default
	* abstract subclass of {@code DragGestureRecognizer}

8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed #17242

8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed #17242

Conversation

pavelrappo commented Jan 3, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 3, 2024

openjdk bot commented Jan 3, 2024

mlbridge bot commented Jan 3, 2024

Webrevs

openjdk bot commented Jan 3, 2024 • edited Loading

Martin-Buchholz Jan 3, 2024

Choose a reason for hiding this comment

pavelrappo Jan 3, 2024

Choose a reason for hiding this comment

Martin-Buchholz Jan 4, 2024

Choose a reason for hiding this comment

pavelrappo Jan 4, 2024

Choose a reason for hiding this comment

pavelrappo commented Jan 9, 2024

openjdk bot commented Jan 9, 2024

openjdk bot commented Jan 9, 2024

magicus commented Jan 11, 2024

pavelrappo commented Jan 11, 2024

pavelrappo commented Jan 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jan 3, 2024 •

edited

Loading