On Wed, 7 Sep 2022 14:40:23 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 Max and Sean This is my last part of code review. Most trivial, but the one in `ssl/ECDHClientKeyExchange.java` might need some attention. src/java.base/share/classes/sun/security/ssl/ECDHClientKeyExchange.java line 289: > 287: } > 288: > 289: // Can't figure this out, bail. Maybe we should instead check if `namedParams` is null on line 286 and avoid the NPE there. src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 70: > 68: @SuppressWarnings({"unchecked", "rawtypes"}) > 69: B_NULL("NULL", NULL_CIPHER, 0, 0, 0, 0, true, true, > 70: new Map.Entry[] { It looks cleaner to me if `new Map.Entry[] {` is put at the end of the previous line. Otherwise, the indentation seems growing backwards. Same for line 80 and all below. src/java.base/share/classes/sun/security/ssl/SSLExtensions.java line 307: > 305: // extension_data > length(2) > 306: } > 307: encodedLength += encoded.length + 4; I think the two comment lines above should follow. src/java.base/share/classes/sun/security/ssl/SSLHandshakeBinding.java line 2: > 1: /* > 2: * Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights > reserved. No change in this file? src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 587: > 585: for (String string : strings) { > 586: builder.append(" \"" + string + "\""); > 587: if (!Objects.equals(string, strings[strings.length - > 1])) { The original usage looks weird. I'd rather use int len = strings.length; for (int i = 0; i < len; i++) { String string = strings[i]; builder.append(" "" + string + """); if (i != len - 1) { builder.append(","); } builder.append("\n"); } src/java.base/share/classes/sun/security/ssl/SSLSocketFactoryImpl.java line 86: > 84: @Override > 85: public Socket createSocket(String host, int port) > 86: throws IOException { Join this with the previous line. src/java.base/share/classes/sun/security/ssl/TransportContext.java line 587: > 585: boolean useUserCanceled = !isNegotiated && > 586: (handshakeContext != null) && !peerUserCanceled; > 587: // initial handshake Move this comment line above a little. ------------- PR: https://git.openjdk.org/jdk/pull/9972
