Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263658: Use the blessed modifier order in java.base #2993

Closed
wants to merge 1 commit into from

Conversation

alblue
Copy link
Contributor

@alblue alblue commented Mar 13, 2021

Sonar displays a warning message that modifiers should be declared in the order listed in the JLS; specifically, that isntead of using final static the static final should be preferred.

This fixes the issues in the java.base package for ease of reviewing.

https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&resolved=false&rules=java%3AS1124


Progress

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

Issue

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

Reviewers

Download

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

To update a local copy of the PR:
$ git checkout pull/2993
$ git pull https://git.openjdk.java.net/jdk pull/2993/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 13, 2021

👋 Welcome back alblue! 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
Copy link

openjdk bot commented Mar 13, 2021

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

  • core-libs
  • hotspot-compiler
  • i18n
  • net
  • 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.

@openjdk openjdk bot added security hotspot-compiler core-libs net i18n labels Mar 13, 2021
@alblue
Copy link
Contributor Author

alblue commented Mar 13, 2021

/cc @shipilev

@openjdk
Copy link

openjdk bot commented Mar 13, 2021

@alblue The label @shipilev is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@shipilev
Copy link
Contributor

shipilev commented Mar 16, 2021

Please change the synopsis to "8263658: Use the blessed modifier order in java.base" to get this PR hooked properly to the new bug. Also configure the run the testing, see "Pre-submit test status" in "Checks".

@alblue alblue changed the title Reorder final static to static final 8263658: Use the blessed modifier order in java.base Mar 17, 2021
@openjdk openjdk bot added the rfr label Mar 17, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 17, 2021

Webrevs

@openjdk
Copy link

openjdk bot commented Mar 17, 2021

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

8263658: Use the blessed modifier order in java.base

Reviewed-by: rriggs, redestad

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

  • 701fd9d: 8262476: Add filter to speed up CompileCommand lookup
  • 454af87: 8263185: Mallinfo deprecated in glibc 2.33
  • d24e4cf: 8263481: Specification of JComponent::setDefaultLocale doesn't mention that passing 'null' restores VM's default locale
  • 1a21f77: 8263482: Make access to the ICC color profiles data multithread-friendly
  • d185655: 8263832: Shenandoah: Fixing parallel thread iteration in final mark task
  • 434a399: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes
  • 6aa28b3: 8263827: Suspend "missing" javadoc doclint checks for smartcardio
  • ed1e25d: 8263833: Stop disabling warnings for sunFont.c with gcc
  • 788e30c: 8263320: [test] Add Object Stream Formatter to work with test utility HexPrinter
  • fa0f161: 8263742: (bf) MappedByteBuffer.force() should use the capacity as its upper bound
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/f9f2eef91ddcff5b5d113577a6c7e520875e77e1...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @cl4es) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Mar 17, 2021
@alblue
Copy link
Contributor Author

alblue commented Mar 17, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 17, 2021
@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@alblue
Your change (at version 470f7066efb476b4f29105a167d46a75d0988a8b) is now ready to be sponsored by a Committer.

cl4es
cl4es approved these changes Mar 18, 2021
@@ -49,7 +49,7 @@
* If the domain provided by the client does not match the one received
* from server.
*/
//public final static int DOMAIN_UNMATCH = 3;
//public static final int DOMAIN_UNMATCH = 3;
Copy link
Member

@cl4es cl4es Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this one ought to be removed instead?

Copy link
Contributor

@shipilev shipilev Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree.

Copy link
Contributor Author

@alblue alblue Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that there to indicate a placeholder value that was once used and is kept for documentation purposes? Should the corresponding JavaDoc be removed as well? Should I do this in the same commit/PR as this one, or submit a new PR? Would prefer to avoid conflating fixes if possible so that if one needs to be reverted we don't revert the related changes.

Copy link
Member

@cl4es cl4es Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another constant with value 3 defined, so I think this is just some left-over.

If you prefer separating out the removal to another RFE I'd remove this particular change from this PR.

Copy link
Contributor Author

@alblue alblue Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #3076 for the removal and updated this PR without it

@alblue
Copy link
Contributor Author

alblue commented Mar 18, 2021

If I have other fixes for different modules, should I file PRs with the same bug number e.g. "8263658: Use the blessed modifier order in java.logging/java.desktop" or should we have separate bug numbers for them?

@openjdk openjdk bot removed the sponsor label Mar 18, 2021
@cl4es
Copy link
Member

cl4es commented Mar 18, 2021

If I have other fixes for different modules, should I file PRs with the same bug number e.g. "8263658: Use the blessed modifier order in java.logging/java.desktop" or should we have separate bug numbers for them?

Separate bug numbers is necessary. Unless you repurpose / widen this PR to include more modules.

A word of advice: Avoid git rebase + force push after opening a PR for review, since it might mess up the review context. Preferably either merge in changes from main, or just keep adding commits without syncing up - the system will ensure your patch can be merged in without conflicts.

@mlbridge
Copy link

mlbridge bot commented Mar 18, 2021

Mailing list message from Joe Darcy on core-libs-dev:

On 3/18/2021 9:53 AM, Alex Blewitt wrote:

On Thu, 18 Mar 2021 14:50:43 GMT, Claes Redestad <redestad at openjdk.org> wrote:

Sonar displays a warning message that modifiers should be declared in the order listed in the JLS; specifically, that isntead of using `final static` the `static final` should be preferred.

This fixes the issues in the `java.base` package for ease of reviewing.

https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&resolved=false&rules=java%3AS1124
Marked as reviewed by redestad (Reviewer).
If I have other fixes for different modules, should I file PRs with the same bug number e.g. "8263658: Use the blessed modifier order in java.logging/java.desktop" or should we have separate bug numbers for them?

Separate bug numbers please. It is also possible to use an umbrella
issue and have issues for, say each module, be subtasks.

-Joe

@alblue
Copy link
Contributor Author

alblue commented Mar 18, 2021

I'm happy to either widen the scope of this PR or to submit multiple but I have to rely on the kindness of strangers to create appropriate bugs in the system. On the one hand I don't want to cause a lot of noise but on the other having smaller independent commits (particularly if they hit a lot of files/modules) seems like a more sensible plan to me.

@cl4es
Copy link
Member

cl4es commented Mar 18, 2021

There's some extra churn in splitting it up, sure, but different modules are often maintained by different people so getting changes that span the entire JDK reviewed might actually take you longer. YMMV. I can assist creating RFEs.

cl4es
cl4es approved these changes Mar 18, 2021
@alblue
Copy link
Contributor Author

alblue commented Mar 19, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 19, 2021
@openjdk
Copy link

openjdk bot commented Mar 19, 2021

@alblue
Your change (at version 86aa9a3) is now ready to be sponsored by a Committer.

@cl4es
Copy link
Member

cl4es commented Mar 19, 2021

/sponsor

@openjdk openjdk bot closed this Mar 19, 2021
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Mar 19, 2021
@openjdk
Copy link

openjdk bot commented Mar 19, 2021

@cl4es @alblue Since your change was applied there have been 47 commits pushed to the master branch:

  • 1572f3c: 8263852: Unused method SoftRefPolicy::use_should_clear_all_soft_refs
  • 57497ab: 8263821: Remove unused MethodTypeForm canonicalization codes
  • 4d51a82: 8263818: Release JNI local references in get/set-InetXXAddress-member helper functions of net_util.c
  • 701fd9d: 8262476: Add filter to speed up CompileCommand lookup
  • 454af87: 8263185: Mallinfo deprecated in glibc 2.33
  • d24e4cf: 8263481: Specification of JComponent::setDefaultLocale doesn't mention that passing 'null' restores VM's default locale
  • 1a21f77: 8263482: Make access to the ICC color profiles data multithread-friendly
  • d185655: 8263832: Shenandoah: Fixing parallel thread iteration in final mark task
  • 434a399: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes
  • 6aa28b3: 8263827: Suspend "missing" javadoc doclint checks for smartcardio
  • ... and 37 more: https://git.openjdk.java.net/jdk/compare/f9f2eef91ddcff5b5d113577a6c7e520875e77e1...master

Your commit was automatically rebased without conflicts.

Pushed as commit b49c589.

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

@alblue alblue deleted the Sonar-BaseModifiers branch Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs hotspot-compiler i18n integrated net security
4 participants