On Thu, 14 Mar 2024 20:30:58 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Change `Krb5LoginModule` debugging to use `sun.security.util.Debug`. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Mark's comments Looks good. Few minor comments made. the new Debug.of method will need to implement some logic from the decorator debug PR at https://github.com/openjdk/jdk/pull/18084 -- depending on integration order, we can tackle that as needed. src/java.base/share/classes/sun/security/util/Debug.java line 172: > 170: * settings. For example, > 171: * {@snippet lang=java: > 172: * Map<String,String> settings = loadLoginSettings(); minor nit - needs a space : `Map<String, String>` src/java.base/share/classes/sun/security/util/Debug.java line 174: > 172: * Map<String,String> settings = loadLoginSettings(); > 173: * String property = settings.get("login"); > 174: * Debug debug = Debug.of("login", setting); did you mean to use the `property` variable name here ? src/java.base/share/classes/sun/security/util/Debug.java line 180: > 178: * @return a new Debug object if the property is true > 179: */ > 180: public static Debug of(String option, String property) { the `property` name is a bit confusing here IMO. Most use cases will be string corresponding to a boolean (or null) - Would a boolean paramater make more sense ? Otherwise. I'd suggest renaming variable to something like `debugEnabled` ------------- PR Review: https://git.openjdk.org/jdk/pull/18199#pullrequestreview-1938907146 PR Review Comment: https://git.openjdk.org/jdk/pull/18199#discussion_r1526259059 PR Review Comment: https://git.openjdk.org/jdk/pull/18199#discussion_r1526259909 PR Review Comment: https://git.openjdk.org/jdk/pull/18199#discussion_r1526262473
