On Thu, 3 Oct 2024 19:09:28 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Weijun Wang has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - Merge branch 'master' into 8340327 >> - more test, more RAW support, fix a bug on cleaning up getRawBytes output >> - add support for private class RawKeySpec >> - ensure key is intact after being used >> - renames >> - the fix > > src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 39: > >> 37: import java.security.ProviderException; >> 38: import java.security.spec.NamedParameterSpec; >> 39: > > Class description? I can add some. This is not public APIs after all. > src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 48: > >> 46: private final byte[] h; >> 47: >> 48: /// Ctor from family name, parameter set name, raw key bytes. > > Any reason you use 3 slashes instead of 2 for comments? I'm trying out the https://openjdk.org/jeps/467 style javadoc. Still, this is not public API so I can take the risk for the experiment. > src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 56: > >> 54: this.algid = AlgorithmId.get(pname); >> 55: } catch (NoSuchAlgorithmException e) { >> 56: throw new ProviderException(e); > > Can this ever happen? If we can be sure that `KnownOIDs` have the mapping then no. I do think this is not a programming error but an inconsistency inside the provider, therefore I choose `ProviderException` instead of `AssertionError`. > src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 67: > >> 65: this.fname = fname; >> 66: decode(encoded); >> 67: paramSpec = new NamedParameterSpec(algid.getName()); > > Use `this` to be consistent with other ctor. Sure. > src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 71: > >> 69: throw new InvalidKeyException("algorithm identifier has >> params"); >> 70: } >> 71: h = getKey().toByteArray(); > > Use this to be consistent with other ctor. Yes. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786761967 PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786762767 PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786764211 PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786763140 PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786763408
