8265426: Update java.security to use instanceof pattern variable #3687

pconcannon · 2021-04-26T08:50:36Z

Hi,

Could someone please review my code for updating the code in the java.security package to make use of the instanceof pattern variable?

Kind regards,
Patrick

Progress

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

Issue

JDK-8265426: Update java.security to use instanceof pattern variable

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)
Weijun Wang (@wangweij - Reviewer)
Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3687

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

Using diff file

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

bridgekeeper · 2021-04-26T08:51:09Z

👋 Welcome back pconcannon! 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 · 2021-04-26T08:54:43Z

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

security

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 · 2021-04-26T08:59:43Z

Webrevs

jespersm

Changes look good, except for the unneeded parenthesis?

(but I'm not a committer, so it's less useful)

src/java.base/share/classes/java/security/Provider.java

src/java.base/share/classes/java/security/CodeSource.java

openjdk · 2021-04-26T18:24:54Z

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

8265426: Update java.security to use instanceof pattern variable

Reviewed-by: rriggs, weijun, dfuchs

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

9a19a0c: 8264760: JVM crashes when two threads encounter the same resolution error
14f0afe: 8214237: Join parallel phases post evacuation
2798b0d: 8266349: Pass down requested page size to reserve_memory_special
e0c8688: 8262992: Improve @see output
d2b5350: 8263507: Improve structure of package summary pages
a65021e: 8266618: Remove broken -XX:-OptoRemoveUseless
94c6177: 8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps
71b8ad4: 8266609: AArch64: include FP/LR space in LIR_Assembler::initial_frame_size_in_bytes()
ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
3a474d9: 8265612: revise the help info for jmap histo command
... and 9 more: https://git.openjdk.java.net/jdk/compare/e8405970b9998ff8f77bcf196f1456713a98c47f...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.

wangweij

Two comments:

Why not reuse the existing variable name (Ex: t in Type t = (Type)obj) as much as possible to avoid unnecessary renames?
I'm not sure if modifying argument name in a public API is a good idea. This leads to unnecessary javadoc changes.

Punikekk

Hi @Punikekk, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user Punikekk for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

src/java.base/share/classes/java/security/Identity.java

Punikekk · 2021-04-27T04:38:32Z

src/java.base/share/classes/java/security/Identity.java

+        if (obj instanceof Identity other) {
+            if (this.fullName().equals(other.fullName())) {
                return true;
            } else {


Suggested change

if (obj instanceof Identity other) {

if (this.fullName().equals(other.fullName())) {

return true;

} else {

Not really sure what you are suggesting here. Could you clarify?

pconcannon · 2021-05-06T11:54:06Z

Two comments:

1. Why not reuse the existing variable name (Ex: `t` in `Type t = (Type)obj`) as much as possible to avoid unnecessary renames?

2. I'm not sure if modifying argument name in a public API is a good idea. This leads to unnecessary javadoc changes.

I initially took the approach you suggested, but the general feedback from previous PRs (on similar work in other areas of java.base) has been to use more descriptive names for the pattern variables where possible. However, if you feel strongly about it please let me know which ones you would like me to change and we can discuss it further.
OK, I thought it might be an opportunity to add in some consistency for parameter names for the equals() methods. But, point taken. I've reverted these changes. Please see commit 9e9f9fb

wangweij

Approved. Just some tiny comments. The other suggestion from @Punikekk also looks fine. Thanks.

src/java.base/share/classes/java/security/cert/CertPath.java

src/java.base/share/classes/java/security/spec/EllipticCurve.java

…thod

pconcannon · 2021-05-07T09:24:15Z

Changes look good, except for the unneeded parenthesis?

(but I'm not a committer, so it's less useful)

Hi Jesper, thanks for your suggestions and well spotted. I've addressed the issues and responded to your comments with the commit id where the changes were made

pconcannon · 2021-05-07T13:41:06Z

/integrate

openjdk · 2021-05-07T13:43:01Z

@pconcannon Since your change was applied there have been 20 commits pushed to the master branch:

3fcdc50: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules
9a19a0c: 8264760: JVM crashes when two threads encounter the same resolution error
14f0afe: 8214237: Join parallel phases post evacuation
2798b0d: 8266349: Pass down requested page size to reserve_memory_special
e0c8688: 8262992: Improve @see output
d2b5350: 8263507: Improve structure of package summary pages
a65021e: 8266618: Remove broken -XX:-OptoRemoveUseless
94c6177: 8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps
71b8ad4: 8266609: AArch64: include FP/LR space in LIR_Assembler::initial_frame_size_in_bytes()
ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
... and 10 more: https://git.openjdk.java.net/jdk/compare/e8405970b9998ff8f77bcf196f1456713a98c47f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 86b8dc9.

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

8265426: Update java.security to use instanceof pattern variable

d435424

openjdk bot added the rfr Pull request is ready for review label Apr 26, 2021

openjdk bot added the security security-dev@openjdk.org label Apr 26, 2021

jespersm reviewed Apr 26, 2021

View reviewed changes

src/java.base/share/classes/java/security/Provider.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/security/Provider.java Outdated Show resolved Hide resolved

RogerRiggs approved these changes Apr 26, 2021

View reviewed changes

src/java.base/share/classes/java/security/CodeSource.java Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Apr 26, 2021

wangweij reviewed Apr 26, 2021

View reviewed changes

Punikekk reviewed Apr 27, 2021

View reviewed changes

pconcannon added 2 commits May 6, 2021 12:16

Merge remote-tracking branch 'origin/master' into JDK-8265426

29308d6

8265426: Reverted parameter names; removed redundant parenthesis

9e9f9fb

wangweij approved these changes May 6, 2021

View reviewed changes

src/java.base/share/classes/java/security/cert/CertPath.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/security/spec/EllipticCurve.java Outdated Show resolved Hide resolved

RogerRiggs approved these changes May 6, 2021

View reviewed changes

pconcannon added 2 commits May 6, 2021 15:20

Merge remote-tracking branch 'origin/master' into JDK-8265426

bd433d6

8265426: changed order of equals check; refactored Identity.equals me…

cbf2841

…thod

dfuch approved these changes May 6, 2021

View reviewed changes

wangweij approved these changes May 6, 2021

View reviewed changes

RogerRiggs approved these changes May 7, 2021

View reviewed changes

openjdk bot closed this May 7, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 7, 2021

pconcannon deleted the JDK-8265426 branch May 25, 2021 09:36

8265426: Update java.security to use instanceof pattern variable #3687

8265426: Update java.security to use instanceof pattern variable #3687

Uh oh!

Conversation

pconcannon commented Apr 26, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 26, 2021

Uh oh!

openjdk bot commented Apr 26, 2021

Uh oh!

mlbridge bot commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jespersm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangweij left a comment

Choose a reason for hiding this comment

Uh oh!

Punikekk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Punikekk Apr 27, 2021 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pconcannon May 6, 2021

Choose a reason for hiding this comment

Uh oh!

pconcannon commented May 6, 2021

Uh oh!

wangweij left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pconcannon commented May 7, 2021

Uh oh!

pconcannon commented May 7, 2021

Uh oh!

openjdk bot commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

pconcannon commented Apr 26, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 26, 2021 •

edited

Loading

openjdk bot commented Apr 26, 2021 •

edited

Loading

Punikekk left a comment •

edited

Loading

Punikekk Apr 27, 2021 •

edited by bridgekeeper bot

Loading