Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Thu, 6 May 2021 13:41:04 GMT, Weijun Wang wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8265426: Reverted parameter names; removed redundant parenthesis
>
> src/java.base/share/classes/java/security/cert/CertPath.java line 185:
>
>> 183:
>> 184: return other instanceof CertPath that
>> 185: && this.type.equals(that.getType())
>
> I know `type` should never be null but let's change as little as possible by
> using `that.getType().equals(this.type)`.
Ordering swapped around. See cbf2841
> src/java.base/share/classes/java/security/spec/EllipticCurve.java line 176:
>
>> 174: && (field.equals(other.field)
>> 175: && a.equals(other.a)
>> 176: && b.equals(other.b));
>
> I suppose there's no need to put the last 3 checks between "(" and ")".
Parenthesis removed as suggested. See cbf2841
-
PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Thu, 6 May 2021 11:52:15 GMT, Patrick Concannon wrote: >> 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 > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8265426: Reverted parameter names; removed redundant parenthesis Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Thu, 6 May 2021 11:52:15 GMT, Patrick Concannon
wrote:
>> 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
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8265426: Reverted parameter names; removed redundant parenthesis
Approved. Just some tiny comments. The other suggestion from @Punikekk also
looks fine. Thanks.
src/java.base/share/classes/java/security/cert/CertPath.java line 185:
> 183:
> 184: return other instanceof CertPath that
> 185: && this.type.equals(that.getType())
I know `type` should never be null but let's change as little as possible by
using `that.getType().equals(this.type)`.
src/java.base/share/classes/java/security/spec/EllipticCurve.java line 176:
> 174: && (field.equals(other.field)
> 175: && a.equals(other.a)
> 176: && b.equals(other.b));
I suppose there's no need to put the last 3 checks between "(" and ")".
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Mon, 26 Apr 2021 23:24:41 GMT, Weijun Wang wrote: > 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. 1. 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. 2. 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 - PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Mon, 26 Apr 2021 17:03:52 GMT, Jesper Steen Møller
wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8265426: Reverted parameter names; removed redundant parenthesis
>
> src/java.base/share/classes/java/security/Provider.java line 2003:
>
>> 2001: return true;
>> 2002: }
>> 2003: if (!(cap.supportsParameter)) {
>
> Why the extra parens?
Parenthesis removed. See 9e9f9fb
> src/java.base/share/classes/java/security/Provider.java line 2012:
>
>> 2010: ("Parameter must be instanceof Key for engine " +
>> type);
>> 2011: }
>> 2012: if (!(hasKeyAttributes())) {
>
> Also here: Why the parens?
Parenthesis removed. See 9e9f9fb
-
PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
On Mon, 26 Apr 2021 18:14:21 GMT, Roger Riggs wrote: >> Patrick Concannon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8265426: Reverted parameter names; removed redundant parenthesis > > src/java.base/share/classes/java/security/CodeSource.java line 174: > >> 172: // certs must match >> 173: return matchCerts(other, true); >> 174: } > > Can this (160-173) be collapsed to: > > return (obj instanceof CodeSource cs) && Objects.equals(location, > other.location) && matchCerts(cs, true) Code refactored as suggested. See 9e9f9fb - PR: https://git.openjdk.java.net/jdk/pull/3687
Re: RFR: 8265426: Update java.security to use instanceof pattern variable [v3]
> 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 Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8265426: Reverted parameter names; removed redundant parenthesis - Changes: - all: https://git.openjdk.java.net/jdk/pull/3687/files - new: https://git.openjdk.java.net/jdk/pull/3687/files/29308d6e..9e9f9fbd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3687&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3687&range=01-02 Stats: 33 lines in 8 files changed: 5 ins; 11 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/3687.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3687/head:pull/3687 PR: https://git.openjdk.java.net/jdk/pull/3687
