On Thu, 1 Sep 2022 02:26:48 GMT, Mark Powers <mpow...@openjdk.org> wrote:
>> https://bugs.openjdk.org/browse/JDK-8291509 > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > comments from Sean and Max This includes all in `sun.security.provider.certpath`. BTW, sometimes some fields are final and some are not, is it better to put the final ones together? src/java.base/share/classes/sun/security/provider/certpath/BasicChecker.java line 105: > 103: prevPubKey = trustedPubKey; > 104: if (PKIX.isDSAPublicKeyWithoutParams(prevPubKey)) { > 105: // If TrustAnchor is a DSA public key, and it has no > params, it Not a native English speaker, but I found the sentence more fluent without the comma. src/java.base/share/classes/sun/security/provider/certpath/BuildStep.java line 192: > 190: String resultString; > 191: switch (res) { > 192: case POSSIBLE: `case` should be indented here. See http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-indentation. That said, I think this is a good chance to use the switch expressions syntax, i.e `return switch (res)...`. src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java line 339: > 337: idpExt.get > 338: > (IssuingDistributionPointExtension.INDIRECT_CRL).equals > 339: (Boolean.FALSE)) { There are only 2 `Boolean` values, so it's safe to use `==`. Also, I'd like idpExt.get(IssuingDistributionPointExtension.INDIRECT_CRL) == Boolean.FALSE src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java line 572: > 570: if (interimReasonsMask[i] && > 571: !(i < reasonsMask.length && reasonsMask[i])) > 572: { More brace to previous line. src/java.base/share/classes/sun/security/provider/certpath/ForwardBuilder.java line 544: > 542: if (distanceTto1 == distanceTto2) { > 543: return -1; > 544: } else if (distanceTto1 > 0 && distanceTto2 <= 0) { I feel there is no need for `distanceTto1 > 0 &&` in the above line. src/java.base/share/classes/sun/security/provider/certpath/OCSPResponse.java line 703: > 701: * > 702: * @return the {@code ResponderId} from this response or {@code null} > 703: * if no responder ID is in the body of the response e.g. a I'd rather add a closed parenthesis at the end, or at least add a comma before `e.g.` src/java.base/share/classes/sun/security/provider/certpath/PolicyNodeImpl.java line 95: > 93: mChildren = new HashSet<>(); > 94: > 95: mValidPolicy = Objects.requireNonNullElse(validPolicy, ""); I prefer `?:`. src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java line 479: > 477: > 478: switch (type) { > 479: case "LDAP": `case` lines should be indented. ------------- PR: https://git.openjdk.org/jdk/pull/9972
