On Tue, 3 May 2022 01:19:20 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 11 commits: >> >> - sixth iteration >> - Merge >> - fifth iteration >> - Merge >> - fourth iteration >> - third iteration >> - Merge >> - Merge >> - Merge >> - second iteration >> - ... and 1 more: >> https://git.openjdk.java.net/jdk/compare/64225e19...5ce46ffb > > src/java.security.jgss/share/classes/org/ietf/jgss/GSSException.java line 333: > >> 331: public String getMajorString() { >> 332: >> 333: return Objects.requireNonNullElseGet(majorString, () -> >> messages[major - 1]); > > Isn't this too fancy? Inside JDK this method is only used once in > `java.time`. The method name has a `requireNonNull` inside so my > understanding is that the object is mostly not null and the else case is just > a fallback (maybe in an exceptional case?) At least in this class, most > `GSSExceptions` are created without a major string. It's been around since JDK 9. I like the functionality, but I hate the name. It returns the first argument if non-null, otherwise it runs the lambda expression. I'll keep the change if you don't mind - one less thing for IntelliJ to nag about. Thanks for taking time to review this! I've accepted two of your suggestions and will be pushing an update shortly. > src/java.security.jgss/share/classes/sun/security/jgss/GSSHeader.java line > 267: > >> 265: throw new IOException("DerInputStream.getLength(): >> lengthTag=" >> 266: + tmp + ", " >> 267: + "too big."); > > Combine the 2 lines above. Fixed. > src/java.security.jgss/share/classes/sun/security/jgss/krb5/SubjectComber.java > line 154: > >> 152: while (iterator.hasNext()) { >> 153: Object obj = iterator.next(); >> 154: if (!(obj instanceof >> @SuppressWarnings("unchecked")KerberosTicket ticket)) { > > Does not look fluent to me. Maybe add a new line after the annotation? Added a new line. > src/java.security.jgss/share/classes/sun/security/jgss/spnego/SpNegoContext.java > line 876: > >> 874: >> 875: // pass token >> 876: tok = Objects.requireNonNullElseGet(token, () -> new byte[0]); > > Is this how `requireNonNullElseGet` is meant to be used? I think so - return the token, or if the token is null, return an empty byte array. ------------- PR: https://git.openjdk.java.net/jdk/pull/7746
