RFR: 8264681: Use the blessed modifier order in java.security

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264681: Use the blessed modifier order in java.security

Alex Blewitt-2
8264681: Use the blessed modifier order in java.security

-------------

Commit messages:
 - 8264681: Use the blessed modifier order in java.security

Changes: https://git.openjdk.java.net/jdk/pull/3338/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3338&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264681
  Stats: 162 lines in 55 files changed: 0 ins; 0 del; 162 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3338.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3338/head:pull/3338

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security

Sean Mullan-2
On Sat, 3 Apr 2021 22:09:55 GMT, Alex Blewitt <[hidden email]> wrote:

> 8264681: Use the blessed modifier order in java.security

The rest looks fine, but I would double-check all the copyrights to see if you are modifying any other 3rd-party code than the ones I commented on. Best to leave that code as-is and fix it at the source, if possible.

src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMSignatureMethod.java line 390:

> 388:         }
> 389:
> 390:         public abstract PSSParameterSpec getPSSParameterSpec();

This is 3rd-party (Apache) code. It would be better to change this at Apache. I would leave this one out.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Constants.java line 53:

> 51:
> 52: /**
> 53:  * This class holds only static final member variables that are constants

Probably best to leave this file unchanged as it is 3rd-party code.

-------------

Marked as reviewed by mullan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security

Aleksey Shipilev-5
In reply to this post by Alex Blewitt-2
On Sat, 3 Apr 2021 22:09:55 GMT, Alex Blewitt <[hidden email]> wrote:

> 8264681: Use the blessed modifier order in java.security

I think some review comments from Sean were left unaddressed...

-------------

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security

Aleksey Shipilev-5
In reply to this post by Sean Mullan-2
On Thu, 8 Apr 2021 17:02:20 GMT, Sean Mullan <[hidden email]> wrote:

>> 8264681: Use the blessed modifier order in java.security
>
> src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMSignatureMethod.java line 390:
>
>> 388:         }
>> 389:
>> 390:         public abstract PSSParameterSpec getPSSParameterSpec();
>
> This is 3rd-party (Apache) code. It would be better to change this at Apache. I would leave this one out.

@alblue, this is the unresolved comment. Please fix it?

> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Constants.java line 53:
>
>> 51:
>> 52: /**
>> 53:  * This class holds only static final member variables that are constants
>
> Probably best to leave this file unchanged as it is 3rd-party code.

Same here...

-------------

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security

Alex Blewitt-2
In reply to this post by Alex Blewitt-2
On Sat, 3 Apr 2021 22:09:55 GMT, Alex Blewitt <[hidden email]> wrote:

> 8264681: Use the blessed modifier order in java.security

Sorry, thought those changes had been removed. Let me fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security [v2]

Alex Blewitt-2
In reply to this post by Alex Blewitt-2
> 8264681: Use the blessed modifier order in java.security

Alex Blewitt has updated the pull request incrementally with one additional commit since the last revision:

  Removed upstream licensed code from commit

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3338/files
  - new: https://git.openjdk.java.net/jdk/pull/3338/files/d91bdba9..3e9047f7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3338&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3338&range=00-01

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3338.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3338/head:pull/3338

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security [v2]

Alex Blewitt-2
On Mon, 12 Apr 2021 09:32:13 GMT, Alex Blewitt <[hidden email]> wrote:

>> 8264681: Use the blessed modifier order in java.security
>
> Alex Blewitt has updated the pull request incrementally with one additional commit since the last revision:
>
>   Removed upstream licensed code from commit

Did a search of the files included in the commit, didn't see anything other than the two above without the standard Oracle copyright .

Wasn't sure about `src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_ATTRIBUTE.java` though; it has an Oracle copyright on it (and not the 'do not alter') one. Should I remove that one from this commit as well?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3338
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264681: Use the blessed modifier order in java.security [v2]

Sean Mullan-2
On Mon, 12 Apr 2021 10:12:06 GMT, Alex Blewitt <[hidden email]> wrote:

> Did a search of the files included in the commit, didn't see anything other than the two above without the standard Oracle copyright .
>
> Wasn't sure about `src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/CK_ATTRIBUTE.java` though; it has an Oracle copyright on it (and not the 'do not alter') one. Should I remove that one from this commit as well?

This one should be ok. We have already made changes to this file so preserving pure parity with the 3rd party sources is not an overriding concern.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3338