On Wed, 15 Apr 2026 04:58:57 GMT, Valerie Peng <[email protected]> wrote:
>> Initial commit containing the public API changes and related regression >> tests. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > additional data cleanup. Comments on APIs. Also, please describe whether the byte array is copied if it is an argument or the return type of any method. Also, do we need more words to explain what `tagLen` means? src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 1: > 1: /* When you add a brand new algorithm, you might also want to update the `info` string and the comment of this class. Or, maybe it's not worth listing them in the comment. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 46: > 44: * <p> This class can be used to initialize a {@code KDF} object that > 45: * implements the <i>Argon2</i> family of algorithms, i.e. > <code>Argon2i</code>, > 46: * <code>Argon2d</code>, and <code>Argon2id</code>. Does not sound right. An object cannot implement an algorithm. Even if so, it can only implement one of the algorithms. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 48: > 46: * <code>Argon2d</code>, and <code>Argon2id</code>. > 47: * > 48: * @since 27 Add a `@spec` tag here for the RFC. Most of the existing tags use the `https://www.rfc-editor.org/info/rfc` format. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 54: > 52: > 53: /** > 54: * Version of Argon2 implementations This is not about the version of an implementation. Maybe you meant a variant? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 62: > 60: V10(0x10), > 61: /** > 62: * version 1.3, the official Argon2 version How official is it? Maybe mention the RFC? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 82: > 80: * {@return V10 for 16, and V13 for 19} > 81: * @param value the integer value as a {@code String} > 82: */ Do you want to add "@throws ISE"? The rendered javadoc shows a "Throws" item without a reason. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 83: > 81: * @param value the integer value as a {@code String} > 82: */ > 83: public static Version get(String value) Why use `String` here, which is inconsistent with the `value` method. If you keep using string, the spec might need to quote the numbers 13 and 19, to make them string like. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 101: > 99: * <p> > 100: * The {@code Builder} is initialized via the {@code newBuilder} > method of > 101: * {@code Argon2ParameterSpec}. As stated in the class description, I know "the class" here means `Argon2ParameterSpec`, but in the rendered HTML, this line is on its own page for `Builder`. Hopefully no one will find it confused. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 102: > 100: * The {@code Builder} is initialized via the {@code newBuilder} > method of > 101: * {@code Argon2ParameterSpec}. As stated in the class description, > 102: * required parameters must be supplied by calling various methods. > Finally, "Various" seems vague. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 168: > 166: * Set the memory value to the builder. > 167: * > 168: * @param m the memory value in Kibibytes I know KiB but this is the first time I see the "kibibyte" word. Maybe some explanation? And, must its first letter be capitalized? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 168: > 166: * Set the memory value to the builder. > 167: * > 168: * @param m the memory value in Kibibytes What does "memory value" mean? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 228: > 226: > 227: /** > 228: * Set tagLen value to the builder. You want to use `tagLen` directly here? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 286: > 284: /** > 285: * Use the specified {@code salt}, {@code password} and the > supplied > 286: * parameters to create an Argon2ParameterSpec object. Quote "Argon2ParameterSpec" inside `{@code}`. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 308: > 306: /** > 307: * Convert {@code passwdChar} to {@code byte[]} based on the > 308: * {@code cs}. Then use it with the specified {@code salt} to No need "the" before "cs". Do you also need to include "and the supplied parameters" to be consistent with the other `build` method? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 346: > 344: // password bytes, len >= 0; clearable thus non-final > 345: private byte[] passwd; > 346: // salt, min len = 8 per argon2-specs.pdf; should be unique for each Is there a full URL for this PDF file? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 416: > 414: > 415: /** > 416: * {@return the parallelism value, 1...2^24-1} Use English. :-) src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 430: > 428: > 429: /** > 430: * {@return the version of Argon2 implementation} I mentioned version of an implementation is an earlier comment. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 437: > 435: > 436: /** > 437: * {@return the optional secret value, byte[0] if not used} What is `byte[0]`. Also, is that an algorithm detail? Should you return an `Optional`? ------------- PR Review: https://git.openjdk.org/jdk/pull/29597#pullrequestreview-4129154796 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100616113 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100855196 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100765952 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100902762 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100920969 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100952202 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3100928220 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101195305 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101210470 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101231624 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101247238 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101253388 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101277711 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101285899 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101313524 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101317202 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101324374 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3101338603
