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

[Christian Hagedorn]

On Mon, 31 Mar 2025 08:57: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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8352869-Verify-NaN-Vector-Objects
>  - Verify.Options refactor for Galder
>  - Update test/hotspot/jtreg/compiler/lib/verify/Verify.java
>    
>    Co-authored-by: Galder Zamarreño <galder at redhat.com>
>  - Merge branch 'master' into JDK-8352869-Verify-NaN-Vector-Objects
>  - clean up test
>  - JDK-8352869

Nice extensions! Some initial comments.

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

> 23: 
> 24: package compiler.lib.verify;
> 25: 

You should update the copyright year.

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

> 35: /**
> 36:  * The {@link Verify} class provides {@link Verify#checkEQ}, which recursively compares the two
> 37:  * {@link Object}s by value. It deconstructs {@link Object[]}, compares boxed primitive types,

Can be any object array right? I would instead write:
Suggestion:

 * {@link Object}s by value. It deconstruct an array of objects, compares boxed primitive types,

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

> 41:  *
> 42:  * <p>
> 43:  * When a comparison fail, then methods print helpful messages, before throwing a {@link VerifyException}.

Suggestion:

 * When a comparison fails, then methods print helpful messages, before throwing a {@link VerifyException}.

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

> 44:  *
> 45:  * <p>
> 46:  * We have to take special care of {@link Float}s and {@link Double}s, since they have both various

Suggestion:

 * We have to take special care of {@link Float}s and {@link Double}s, since they both have various

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

> 45:  * <p>
> 46:  * We have to take special care of {@link Float}s and {@link Double}s, since they have both various
> 47:  * encodings for NaN values, but on Java specification they are to be regarded as equal. Hence, we

Suggestion:

 * encodings for NaN values while the Java specification regards them as equal. Hence, we

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

> 66: 
> 67:         /**
> 68:          * Generates a {@link Options} with default settings.

Suggestion:

         * Generates an {@link Options} object with default settings.

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

> 84:          * By default, we only support the comparison of a limited set of types, but with this option
> 85:          * enabled, we can compare arbitrary classes by value, and we compare the Objects by
> 86:          * the recursive structore given by their field values.

Suggestion:

         * the recursive structure given by their field values.

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

> 207:                     print(a, b, field, aParent, bParent);
> 208:                     throw new VerifyException("Object type not supported: " + ca.getName() + " -- did you mean to 'enableCheckWithArbitraryClasses'?");
> 209:                 }

What's the reason behind throwing instead of just comparing two arbitrary objects by default? If a user calls `Verify.checkEQ()` and sees this exception, I would guess he then just passes the additional option and we have the same result. But maybe I'm missing something.

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

PR Review: https://git.openjdk.org/jdk/pull/24224#pullrequestreview-2729239101
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021016321
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021029237
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021029655
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021030792
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021032586
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021039554
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021040835
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2021108161