RFR: 8352869: Verify.checkEQ: extension for NaN, VectorAPI and arbitrary Objects [v7]

[Emanuel Peter]

On Wed, 2 Apr 2025 14:57:12 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   refactor with checkEQWithRawBits
>
> Thanks for the update! It's much easier to use and understand now I think.
> 
> I did a complete pass and left a lot of comments but mostly minor things. Overall, I think this looks great! :-)

@chhagedorn Thanks for the thorough review :)
I think I addressed all your comments 😊

> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 236:
> 
>> 234:      * pattern in all cases, except for NaN we project to the canonical NaN, using Float.floatToIntBits.
>> 235:      */
>> 236:     private boolean isFloatEQ(float a, float b) {
> 
> Shouldn't this be named `isFloatNotEQ` since you return true when they are different? Same for `isDoubleEQ` below. Alternatively: Return true when they are equal (i.e. flip condition).

Good catch! Flipped the condition :)

> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 316:
> 
>> 314:      * Verify that the content of two MemorySegments is identical. Note: we do not check the
>> 315:      * backing type, only the size and content.
>> 316:      */
> 
> Probably a copy-paste error. Should be updated for exceptions.

Good catch, updated it!

> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 463:
> 
>> 461:     private void print(Object a, Object b, String field, Object aParent, Object bParent) {
>> 462:         System.err.println("  aParent: " + aParent);
>> 463:         System.err.println("  bParent: " + bParent);
> 
> Should we print `null` parents or just skip them?

I think it does not hurt to print `null` here. It makes the code a little simpler.

> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 520:
> 
>> 518:         long start = Long.max(offset - range, 0);
>> 519:         long end   = Long.min(offset + range, a.byteSize());
>> 520:         for (long i = start; i < end; i++) {
> 
> Nit below: You can replace `System.err.println("")` with `System.err.println()`.

done!

-------------

PR Comment: https://git.openjdk.org/jdk/pull/24224#issuecomment-2774605365
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026279491
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026276939
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026280168
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026281282