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

[Galder Zamarreño]

On Tue, 25 Mar 2025 11:15:58 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.

Changes requested by galder (Author).

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

> 54:  * <p>
> 55:  * By default, we only support comparison of the types mentioned above. However, in some cases one
> 56:  * might want to compare Objects of arbitrare classes by value, i.e. the recursive structure given

Suggestion:

 * might want to compare Objects of arbitrary classes by value, i.e. the recursive structure given

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

> 77:      * @throws VerifyException If the comparison fails.
> 78:      */
> 79:     public static void checkEQ(Object a, Object b, boolean isFloatCheckWithRawBits, boolean isCheckWithArbitraryClasses) {

Just a suggestion. One boolean might be ok, but once you start adding 2 booleans it seems like a bit of a code smell to me. Do you envision more options being added? I would personally create a `VerifyOptions` record with the boolean flags options and pass that in.

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

> 262:     private void checkEQimpl(float a, float b, String field, Object aParent, Object bParent) {
> 263:         if (isFloatEQ(a, b)) {
> 264:             System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits);

Would using a text block here make it more readable?

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

> 275:     private void checkEQimpl(double a, double b, String field, Object aParent, Object bParent) {
> 276:         if (isDoubleEQ(a, b)) {
> 277:             System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits);

Same text block comment here

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

> 470: 
> 471:     private void print(Object a, Object b, String field, Object aParent, Object bParent) {
> 472:         System.err.println("  aParent: " + aParent);

Text block?

test/hotspot/jtreg/testlibrary_tests/verify/tests/TestVerify.java line 691:

> 689:     }
> 690: 
> 691:     public static void checkNE(Object a, Object b) {

If `checkEQ` lives in Verify, wouldn't it make sense to also have `checkNE` there? Seems like the natural place making the API symmetric.

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

PR Review: https://git.openjdk.org/jdk/pull/24224#pullrequestreview-2724270449
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2017921263
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2017924041
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2017926106
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2017926245
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2017927145
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2017930961