On Thu, 9 May 2024 12:22:14 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> some code review comments
>
> src/java.base/share/classes/javax/crypto/KDF.java line 43:
>
>> 41:
>> 42: /**
>> 43: * This class provides the functionality of a key derivation algorithm
>> for the Java Cryptographic
>
> We don't normally say "for the Java Cryptographic Extension framework" in our
> other APIs, so I would remove that part. Also, can you try to keep lines to
> around 80 characters - it helps with code reviews.
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 46:
>
>> 44: * Extension (JCE) framework.
>> 45: * <p>
>> 46: * {@code KeyDerivation} objects will be instantiated through the {@code
>> getInstance} family of
>
> s/KeyDerivation/KDF/
> s/will be/are/
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 47:
>
>> 45: * <p>
>> 46: * {@code KeyDerivation} objects will be instantiated through the {@code
>> getInstance} family of
>> 47: * methods. Key derivation algorithm names will follow a naming
>> convention of
>
> remove "will".
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 48:
>
>> 46: * {@code KeyDerivation} objects will be instantiated through the {@code
>> getInstance} family of
>> 47: * methods. Key derivation algorithm names will follow a naming
>> convention of
>> 48: * <I>algorithm</I>/<I>PRF</I>. The algorithm field will be the KDF name
>
> s/will be/is/
> s/name/algorithm/
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 89:
>
>> 87:
>> 88: /**
>> 89: * Instantiates a KeyDerivation object.
>
> s/KeyDerivation/KDF/
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 100:
>
>> 98: * the algorithm parameters
>> 99: */
>> 100: protected KDF(KDFSpi keyDerivSpi, Provider provider, String
>> algorithm,
>
> This class is final, so a protected constructor is not necessary. You should
> be able to make this private.
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 110:
>
>> 108:
>> 109: /**
>> 110: * Returns the algorithm name of this {@code KeyDerivation} object.
>
> s/KeyDerivation/KDF/
>
> There are others that need to be corrected, search for all other instances in
> this class.
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 115:
>
>> 113: * {@code getInstance} calls that created this {@code
>> KeyDerivation} object.
>> 114: *
>> 115: * @return the algorithm name of this {@code KeyDerivation} object.
>
> Nit: remove period, sentences that don't start with a capital letter and
> don't have a following sentence should not have period at end
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 117:
>
>> 115: * @return the algorithm name of this {@code KeyDerivation} object.
>> 116: */
>> 117: public final String getAlgorithm() {
>
> The class is final, so none of the methods need to have the final keyword.
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/KDF.java line 131:
>
>> 129: }
>> 130:
>> 131: private String getProviderName() {
>
> Make this method public.
Done. Please resolve if satisfied.
> src/java.base/share/classes/javax/crypto/spec/KDFParameterSpec.java line 32:
>
>> 30: *
>> 31: * <P> This interface contains no methods or constants. Its only purpose
>> 32: * is to group (and provide type safety for) all parameter
>> specifications. All KDF parameter
>
> The words "all parameter specifications" is too general as this is specific
> to KDF. I would probably change this to "all KDF parameter specifications."
> Then you don't need the last sentence as it says the same thing.
Done. Please resolve if satisfied.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927487
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927563
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927626
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927676
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927914
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927992
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928123
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928198
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928294
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595928393
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1595927336
