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

[Christian Hagedorn]

On Thu, 3 Apr 2025 06:27:33 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> We should extend the functionality of Verify.checkEQ:
>> - Allow different NaN encodings to be seen as equal (by default).
>> - Compare VectorAPI vectors.
>> - Compare Exceptions, and their messages.
>> - Compare arbitrary Objects via Reflection.
>> 
>> Note: this is a prerequisite for the Template Library [JDK-8352861](https://bugs.openjdk.org/browse/JDK-8352861) / https://github.com/openjdk/jdk/pull/23418.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix whitespace issues

Thanks for addressing all my comments and doing the updates! I have some more final comments but then I think it's good to go from my side!

test/hotspot/jtreg/compiler/lib/verify/Verify.java line 62:

> 60:      * When comparing arbitrary classes recursively, we need to remember which
> 61:      * pairs of objects {@code (a, b)} we have already visited. The maps
> 62:      * {@link a2b} and {@link b2a} track these edges. Caching which pairs

I think it's fine to use `code` here since the Javadocs links to itself otherwise.
Suggestion:

     * {@code a2b} and {@code b2a} track these edges. Caching which pairs

test/hotspot/jtreg/compiler/lib/verify/Verify.java line 77:

> 75:      * Verify the contents of two Objects on a raw bit level, possibly recursively.
> 76:      * Different NaN encodings are considered non-equal, since we compare
> 77:      * floating number by their raw bits.

Suggestion:

     * floating numbers by their raw bits.

test/hotspot/jtreg/compiler/lib/verify/Verify.java line 90:

> 88:     /**
> 89:      * Verify the contents of two Objects, possibly recursively.
> 90:      * Different NaN encodins are considered equal.

Suggestion:

     * Different NaN encodings are considered equal.

test/hotspot/jtreg/compiler/lib/verify/Verify.java line 307:

> 305:      * Verify that two Exceptions have the same message. Messages are not always carried,
> 306:      * they are often dropped to performance, and that is ok. But if both Exceptions have
> 307:      * the message, we should compare them.

Suggestion:

     * they are often dropped for performance reasons, and that is okay. But if both Exceptions
     * have the message, we should compare them.

test/hotspot/jtreg/compiler/lib/verify/Verify.java line 438:

> 436:      * to add "--add-modules=jdk.incubator.vector" to the command-line of every test that uses the Verify
> 437:      * class. So we hack this via reflection.
> 438:      */

I think this background is only needed at `checkEQForVectorAPIClass()` (where you already have that comment). Here you can just describe what the code actually does or just drop the comment entirely since the method name is self-explanatory :-)

test/hotspot/jtreg/compiler/lib/verify/Verify.java line 495:

> 493:      * When comparing arbitrary classes recursively, we need to remember which
> 494:      * pairs of objects {@code (a, b)} we have already visited. The maps
> 495:      * {@link a2b} and {@link b2a} track these edges. Caching which pairs

Suggestion:

     * {@link #a2b} and {@link #b2a} track these edges. Caching which pairs

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

PR Review: https://git.openjdk.org/jdk/pull/24224#pullrequestreview-2738785943
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026406773
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026370608
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026371049
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026396345
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026401231
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026407956