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. Some comments for `Argon2ParameterSpec.java`. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 178: > 176: * if {@code m} is less than 8; or if {@code > parallelism(p)} > 177: * has already been called and {@code m} is less than > 178: * {@code 8 * p} I don't know if this will be a real problem: suppose user first set `m = 8; p = 1` and then decide to switch to `m = 16; p = 2`. They will need to remember to first adjust `m` and then `p` otherwise changing `p` will throw an exception. Maybe no serious user would set `m` as low as 8, but what about `m = 32, p = 4` or more? This is still a problem. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 187: > 185: > 186: /** > 187: * Sets the memory cost to {@code 2}<sup>{@code mPower}</sup> > KiB. Just curious, why should "2" be rendered in `{@code}`? The whole thing is no longer a literal Java expression. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 194: > 192: * if {@code mPower} is less than 3 or greater than 30; > or if > 193: * {@code parallelism(p)} has already been called and > 194: * {@code mPower} is less than {@code 3 + ceil(log2(p))} I find it a little difficult to understand this. Can we say `and 2^mPower is less than 8 * p`? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 243: > 241: > 242: /** > 243: * Sets the Argon2 version. Do we need to mention this is optional or has a default value? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 319: > 317: * or has fewer than 8 bytes; if {@code passwdChar} or > 318: * {@code cs} is {@code null}; or if any required builder > 319: * parameter has not been set. There is a period at the end of this `@throws` but none in the one of the method above. Please be consistent. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 383: > 381: this.t = builder.t; > 382: this.k = builder.k.clone(); > 383: this.x = builder.x.clone(); Do you want to use `B0` when `k` and/or `x` is empty? I see you did it elsewhere. src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 474: > 472: > 473: /** > 474: * Clears the password and the secret value held by this object. I wonder if user would ask how to clear the secret in a `Builder`. Maybe let it implements`Destroyable` is too big. Maybe we can add a sentence in `Builder::secret(byte[] k)` that calling it will clear the previous secret. At least curious users will find a way. ------------- PR Review: https://git.openjdk.org/jdk/pull/29597#pullrequestreview-4997280483 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833695868 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833593198 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833566716 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833723393 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833739487 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833831783 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r3833808809
