On Thu, 4 Sep 2025 16:54:48 GMT, Matthew Donovan <[email protected]> wrote:
>> test/jdk/sun/security/pkcs11/Cipher/TestKATForGCM.java line 330:
>>
>>> 328: String osName = System.getProperty("os.name");
>>> 329: int idx = ver.indexOf(".");
>>> 330: double major = Double.parseDouble(ver.substring(0,
>>> idx));
>>
>> IMO the split between major version and minor is a bit hard to read.
>> Wouldn't it be easier to just get a major.minor version entirely with
>> something like:
>>
>>
>> String[] splitParts = ver.split("//.");
>> Double.parseDouble(splitParts.length > 1
>> ? splitParts[0] + "." + splitParts[1]
>> : splitParts[0]);
>>
>>
>> This way it will always take a full double value (1.13 will not be the same
>> as 1.1, as it's now as far as I can see) and would be a bit easier to
>> understand
>>
>> The checking for only major version could be either `doubleVersion<4 &&
>> doubleVersion>=3` or even cleaner, using floor function
>> `Math.floor(doubleVersion)`
>>
>> And the same for the other files.
>>
>> What do you think?
>
> I may be misunderstanding your comment but it looks like you're using a
> single, `double` value to compare versions. The original code did that but it
> doesn't work in all cases. The problem occurs when checking a range such as
>
> `version >= 3.11 && version < 3.12`
>
> If the version number is 3.111, then the comparison is `true` and tests are
> skipped, even though _version_ 3.111 is "greater" than 3.12. So this code
> creates two doubles: `major` and `minor` to do separate comparisons of the
> values.
>
> To further complicate things, NSS also has versions of the form `x.y.z`. The
> original code combined the 'y' and 'z' components to go from 3.11.9 to 3.119.
> My change would create major version 3.0 and minor version 11.9.
Ah, you're right, I didn't think of this case! This approach will be easier
then, I agree.
Still, `ver.substring(idx+1)` may cause errors in cases like this [NSS 3.15.3.1
release
notes](https://nss-crypto.org/reference/security/nss/legacy/nss_releases/nss_3.15.3.1_release_notes/index.html#mozilla-projects-nss-nss-3-15-3-1-release-notes).
It will try to convert 15.3.1 to a double. Do you think changing to something
like this would be worth it?
String verSubstring = ver.substring(idx+1);
String[] splitParts = verSubstring.split("//.");
Double minor = Double.parseDouble(splitParts.length > 1
? splitParts[0] + "." + splitParts[1]
: splitParts[0]);
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27095#discussion_r2324586473
