On Wed, 7 Sep 2022 01:41:14 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Mark Powers has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> comments from Max and Sean
>
> 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.
Fixed. Throwing exception sooner rather than later.
> 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.
I did as you suggested and did a bit more with the indentation.
> 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.
Sure that looks right?
> 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?
Fixed. No change to 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");
> }
Fixed.
> 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.
Fixed.
> 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.
Fixed.
-------------
PR: https://git.openjdk.org/jdk/pull/9972
