On Wed, 4 Feb 2026 10:20:57 GMT, Mikhail Yankelevich <[email protected]>
wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> refactoring structures
>
> test/jdk/sun/security/provider/acvp/ML_KEM_Test.java line 30:
>
>> 28:
>> 29: import javax.crypto.KEM;
>> 30: import java.security.*;
>
> Minor: could you please remove wildcard import?
Yes, I can.
> test/jdk/sun/security/provider/acvp/ML_KEM_Test.java line 91:
>
>> 89: var function = t.get("function").asString();
>> 90: System.out.println(">> " + pname + " " + function);
>> 91: for (var c : t.get("tests").asArray()) {
>
> Minor: could you please change this var to a class name? It's a bit hard to
> read IMO
I actually don't know the class name. All I know is that in the JSON file,
"tests" points to an array and I believe there is a way to iterate through an
array. I don't care if `c` is a `JSONObject` or a `JSONMap` or a `JSONValue`.
The IDE allows me to use it as a "map"-thingy to drill into its named fields
and that's enough.
> test/jdk/sun/security/provider/acvp/ML_KEM_Test.java line 112:
>
>> 110: g.newEncapsulator(ek);
>> 111: } else {
>> 112: Asserts.assertThrows(Exception.class,
>> () -> g.newEncapsulator(ek));
>
> Do you think it's best to change this to expect a more exact exception?
> Unless I'm mistaken the method `newEncapsulator` throws InvalidKeyException.
> This way the test won't accept an unexpected null pointer for example
Yes, I can.
> test/jdk/sun/security/provider/acvp/ML_KEM_Test.java line 124:
>
>> 122: if (function.equals("decapsulation")) {
>> 123: var d = g.newDecapsulator(dk);
>> 124: var k =
>> d.decapsulate(toByteArray(c.get("c").asString()));
>
> minor: the same with `k` as above, without being familiar with the test it's
> quite hard to read.
Do you want `var` expanded to the concrete type here?
When we designed the KEM API, we intentionally expected callers to use `var`.
The nested types live under `KEM` and have fairly long names
(`KEM.Decapsulator`, `KEM.Encapsulator`, etc.), and the API was structured so
users would not need to remember or spell them explicitly.
Instead of using explicit types, How about I renaming the variables to be more
descriptive?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29548#discussion_r2776903350
PR Review Comment: https://git.openjdk.org/jdk/pull/29548#discussion_r2776911402
PR Review Comment: https://git.openjdk.org/jdk/pull/29548#discussion_r2776927916
PR Review Comment: https://git.openjdk.org/jdk/pull/29548#discussion_r2776927438
