On Thu, 6 Aug 2026 21:35:48 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: > > import cleanup. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 108: > 106: * > 107: * <p>Obtain a {@code Builder} by calling > 108: * {@code Argon2ParameterSpec.newBuilder}. Supply the required > parameters I know you can figure it out by looking for "optional" in the methods, but I think it would also be useful to list the required parameters here. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 221: > 219: * @return this builder > 220: * @throws IllegalArgumentException if {@code p} is not positive, > 221: * greater than {@code 16777215}, or greater than {@code > m / 8} I think it would be useful to note that 16777215 is also 2^(24)-1 per the Argon2 RFC. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 221: > 219: * @return this builder > 220: * @throws IllegalArgumentException if {@code p} is not positive, > 221: * greater than {@code 16777215}, or greater than {@code > m / 8} I don't think it is clear what `m` is. I think you should use similar words as in `memory()` when IAE is thrown because parallelism is too high, ex: "or if `memoryKiB(m)` has been called and `p` is greater than `m / 8`." (Also add similar wording if `memoryPowerOfTwo` is called). test/jdk/javax/crypto/spec/Argon2ParameterSpec/InvalidArgs.java line 65: > 63: final byte[] b8 = "12345678".getBytes(); > 64: final char[] c0 = new char[0]; > 65: // setup the builder w/ the required parameters How about adding a test where the required parameters are not all set? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3806994161 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3806859832 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3806905615 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3806981239
