On Sat, 12 Sep 2026 12:18:57 GMT, Mikhail Yankelevich
<[email protected]> wrote:
>> Mark Powers has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - comment from Weijun to eliminate huge indentation from if statement
>> - comment from Weijun about probe patterns
>
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 2586:
>
>> 2584: (PKCS12_HEADER_PATTERNS[i][2] ==
>> 2585: (finalPeek & PKCS12_HEADER_MASKS[i][2]))) {
>> 2586: result = true;
>
> Nit: Wouldn't it be a bit easier to read if this just returns true and the
> line 2591 returns false?
Yes.
> test/jdk/sun/security/pkcs12/EmptyAuthSafeTest.java line 52:
>
>> 50: private static final String ks2 = "MBACAQMwCwYJKoZIhvcNAQcB";
>> 51:
>> 52: public static void main(String[] args) throws Exception {
>
> This test structure will skip the rest of the test if there is a failure. I
> think it should be either a junit test or the errors should be stored and
> printed in the end.
> Personally, I think junit might be the easiest way. What do you think?
Good idea. I've never used junit but it does seem to be a good fit in this case.
> test/jdk/sun/security/pkcs12/EmptyAuthSafeTest.java line 89:
>
>> 87: boolean actual =
>> PKCS12KeyStore.isPasswordless(keyStoreFile.toFile());
>> 88: if (actual != expected) {
>> 89: throw new Exception("Expected isPasswordless() to return "
>
> Wouldn't `RuntimeException` fit better here and in the other parts of the
> test file? This is what is normally used
unnecessary with junit
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32357#discussion_r4010340580
PR Review Comment: https://git.openjdk.org/jdk/pull/32357#discussion_r4010340027
PR Review Comment: https://git.openjdk.org/jdk/pull/32357#discussion_r4010339483
