On Tue, 27 Jan 2026 23:28:39 GMT, Bradford Wetmore <[email protected]> wrote:
>> Sean Coffey has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 48 commits: >> >> - correct NamedGroup.java merge >> - Merge branch 'master' into 8044609-ssl >> - fix up test bug ID >> - fix up files post merge >> - Merge branch 'master' into 8044609-ssl >> - prep for isOn() merge >> - Merge branch 'master' into 8044609-ssl >> - Merge branch 'master' into 8044609-ssl >> - Merge branch 'master' into 8044609-ssl >> - Incorporate review comments from Brad >> - ... and 38 more: https://git.openjdk.org/jdk/compare/c69275dd...6b5c692c > > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 62: > >> 60: public final class SSLLogger implements System.Logger { >> 61: private static final System.Logger logger; >> 62: // high level boolean to track whether "all" or "ssl" option > > Minor nit: > This makes it sounds like the boolean state `true` might be `all` and `false` > is `ssl`. > > Suggest maybe "to track whether logging is active (i.e. all/ssl)." done > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 105: > >> 103: if (Opt.HANDSHAKE.on && p.contains("verbose")) { >> 104: Opt.HANDSHAKE_VERBOSE.on = true; >> 105: } > > Not absolutely needed given the options currently defined, but you could > `replaceFirst()` on these 3 as well. > > Your call. thanks - I might leave this out for now. Unlikely we'll ever need such values to be removed. > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 130: > >> 128: } >> 129: } >> 130: // javax.net.debug would be misconfigured property with >> respect > > Nit: add extra line. done > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 149: > >> 147: return logging; >> 148: } >> 149: /** > > Nit: add a line. done > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 237: > >> 235: System.err.printf("%nAdding valid filter options to \"ssl\" >> will log" + >> 236: " messages to include%njust those filtered >> categories.%n"); >> 237: System.err.printf("%nIf \"ssl\" is specified by itself," + > > Maybe reverse 235 and 237. ssl by itself, then ssl+filter? I've gone > back/forth on this quite a bit. Do you have a preference? good point. I've swapped these around. I think it reads well now. > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 296: > >> 294: return level != Level.OFF; >> 295: } >> 296: /** > > Nit: add a line. done > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 311: > >> 309: * Enabling subcomponents fine-tunes (filters) debug output. >> 310: */ >> 311: public enum Opt { > > Does this need to be public? Looks like there's some usage of SSLLogger outside of sun.security.ssl e.g. sun/security/util/DomainName.java > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 465: > >> 463: formatParameters(parameters) : >> 464: Utilities.indent(formatParameters(parameters))) >> 465: }; > > Why did the indent get changed? not sure! I've reverted the indentation back > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 490: > >> 488: isFirst = false; >> 489: } else { >> 490: builder.append(",\n"); > > Why did this get changed from `LINE_SEP` to `\n`. All of the other line > separators in this file are still `LINE_SEP`. this might have crept back in during a merge operation. Resolved. > src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 494: > >> 492: >> 493: if (parameter instanceof Throwable) { >> 494: >> builder.append(formatThrowable((Throwable)parameter)); > > Nit: from the Java Coding conventions, Section 8.2: > > Casts should be followed by a blank. Examples: > myMethod((byte) aNum, (Object) x); > myFunc((int) (cp + 5), ((int) (i + 3)) > > I don't have a strong preference here. > > If you do revert back, there are several places that got changed here. Thanks - I need to remember that coding convention. Seen different styles. I've taken an extra step here and introduced instanceof pattern matching where possible. Cleans up the code some more. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737388191 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737392636 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737392983 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737393402 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737395056 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737395731 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737398470 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737400506 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737402184 PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737405959
