On Tue, 7 Jun 2022 15:37:02 GMT, Mark Powers <d...@openjdk.java.net> wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done >> in java.security >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/security > > Mark Powers has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 11 commits: > > - Merge > - fourth iteration > - Merge > - Merge > - third iteration > - Merge > - Merge > - second iteration > - Merge > - Merge > - ... and 1 more: > https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01 Some comments. Thanks for the hard work. src/java.base/share/classes/java/security/MessageDigest.java line 306: > 304: } else { > 305: return Delegate.of((MessageDigestSpi)objs[0], algorithm, > 306: (Provider)objs[1]); Indent this line. src/java.base/share/classes/java/security/ProtectionDomain.java line 474: > 472: return true; > 473: } catch (SecurityException se) { > 474: // fall through and return false How about we just `return false` here and then there is no need for the `return false` at the end? src/java.base/share/classes/java/security/ProtectionDomain.java line 474: > 472: return true; > 473: } catch (SecurityException se) { > 474: // fall through and return false How about we just `return false` here and then there is no need for the `return false` at the end? src/java.base/share/classes/java/security/ProtectionDomain.java line 492: > 490: Policy p = Policy.getPolicyNoCheck(); > 491: return p.getPermissions(ProtectionDomain.this); > 492: }); First, indent these lines. (or maybe my GitHub view has not reveals the indentation?). Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`. src/java.base/share/classes/java/security/ProtectionDomain.java line 492: > 490: Policy p = Policy.getPolicyNoCheck(); > 491: return p.getPermissions(ProtectionDomain.this); > 492: }); First, indent these lines. (or maybe my GitHub view has not reveals the indentation?). Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`. src/java.base/share/classes/java/security/Provider.java line 1098: > 1096: boolean matches(String type, String algorithm) { > 1097: return (Objects.equals(this.type, type)) && > 1098: (Objects.equals(this.originalAlgorithm, algorithm)); In fact, I'm not sure why the original code is brave enough to use `==` instead of `equals`. If you do believe it's a real bug (and can write a regression test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a comment saying it's correct. src/java.base/share/classes/java/security/Provider.java line 1098: > 1096: boolean matches(String type, String algorithm) { > 1097: return (Objects.equals(this.type, type)) && > 1098: (Objects.equals(this.originalAlgorithm, algorithm)); In fact, I'm not sure why the original code is brave enough to use `==` instead of `equals`. If you do believe it's a real bug (and can write a regression test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a comment saying it's correct. src/java.base/share/classes/java/security/SecureClassLoader.java line 221: > 219: // only), and the fragment is not considered. > 220: CodeSourceKey key = new CodeSourceKey(cs); > 221: /* not used */ What is not used? `key1`? How about just rename it to `unused`? src/java.base/share/classes/java/security/SecureClassLoader.java line 221: > 219: // only), and the fragment is not considered. > 220: CodeSourceKey key = new CodeSourceKey(cs); > 221: /* not used */ What is not used? `key1`? How about just rename it to `unused`? src/java.base/share/classes/java/security/SecureClassLoader.java line 235: > 233: } > 234: > 235: private record CodeSourceKey(CodeSource cs) { Really necessary? src/java.base/share/classes/java/security/SecureClassLoader.java line 235: > 233: } > 234: > 235: private record CodeSourceKey(CodeSource cs) { Really necessary? src/java.base/share/classes/java/security/SecureRandom.java line 905: > 903: private static final Pattern pattern = > 904: Pattern.compile( > 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?"); Don't quite understand this change. Can you explain? src/java.base/share/classes/java/security/SecureRandom.java line 905: > 903: private static final Pattern pattern = > 904: Pattern.compile( > 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?"); Don't quite understand this change. Can you explain? src/java.base/share/classes/java/security/Security.java line 260: > 258: } > 259: > 260: return null; If `entry` is always null, then we don't need to declare it at all. src/java.base/share/classes/java/security/Security.java line 260: > 258: } > 259: > 260: return null; If `entry` is always null, then we don't need to declare it at all. src/java.base/share/classes/java/security/Signature.java line 271: > 269: (algorithm + " Signature not available"); > 270: } > 271: // try services until we find a Spi or a working Signature > subclass Is `Spi` pronounced `spy` or "S-P-I"? src/java.base/share/classes/java/security/Signature.java line 271: > 269: (algorithm + " Signature not available"); > 270: } > 271: // try services until we find a Spi or a working Signature > subclass Is `Spi` pronounced `spy` or "S-P-I"? src/java.base/share/classes/java/security/UnresolvedPermission.java line 369: > 367: this.certs != null && that.certs == null || > 368: this.certs != null && > 369: this.certs.length != that.certs.length) { I see you keep a lot of parentheses in other places. For example, `(!r)`. src/java.base/share/classes/java/security/UnresolvedPermission.java line 369: > 367: this.certs != null && that.certs == null || > 368: this.certs != null && > 369: this.certs.length != that.certs.length) { I see you keep a lot of parentheses in other places. For example, `(!r)`. src/java.base/share/classes/java/security/cert/CertPathValidator.java line 335: > 333: String cpvtype = > 334: AccessController.doPrivileged((PrivilegedAction<String>) () > -> > 335: Security.getProperty(CPV_TYPE)); See too many, maybe we need a dedicated method in `sun.security.util.SecurityProperties`? src/java.base/share/classes/java/security/cert/CertPathValidator.java line 335: > 333: String cpvtype = > 334: AccessController.doPrivileged((PrivilegedAction<String>) () > -> > 335: Security.getProperty(CPV_TYPE)); See too many, maybe we need a dedicated method in `sun.security.util.SecurityProperties`? src/java.base/share/classes/java/security/cert/LDAPCertStoreParameters.java line 34: > 32: * name and port number) to implementations of the LDAP {@code CertStore} > 33: * algorithm. However, if you are retrieving certificates or CRLs from > 34: * a ldap URI as specified by RFC 5280, use the Isn't `ldap` pronounced `L-Dap`? src/java.base/share/classes/java/security/cert/LDAPCertStoreParameters.java line 34: > 32: * name and port number) to implementations of the LDAP {@code CertStore} > 33: * algorithm. However, if you are retrieving certificates or CRLs from > 34: * a ldap URI as specified by RFC 5280, use the Isn't `ldap` pronounced `L-Dap`? src/java.base/share/classes/java/security/cert/PKIXBuilderParameters.java line 193: > 191: public String toString() { > 192: return "[\n" + > 193: super.toString() + Is `toString` necessary? src/java.base/share/classes/java/security/cert/PKIXBuilderParameters.java line 193: > 191: public String toString() { > 192: return "[\n" + > 193: super.toString() + Is `toString` necessary? src/java.base/share/classes/java/security/spec/ECPoint.java line 103: > 101: return obj instanceof ECPoint other > 102: && ((Objects.equals(x, other.x)) > 103: && (Objects.equals(y, other.y))); `x` and `y` will not be null unless this is `POINT_INFINITY`, but this is already checked in both `equals` and `hashCode`. src/java.base/share/classes/java/security/spec/ECPoint.java line 103: > 101: return obj instanceof ECPoint other > 102: && ((Objects.equals(x, other.x)) > 103: && (Objects.equals(y, other.y))); `x` and `y` will not be null unless this is `POINT_INFINITY`, but this is already checked in both `equals` and `hashCode`. ------------- PR: https://git.openjdk.java.net/jdk/pull/8319
