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

[Emanuel Peter]

On Mon, 31 Mar 2025 14:04:29 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.

@chhagedorn Thanks for the suggestions and questions! I think I addressed them all :)

> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 25:
> 
>> 23: 
>> 24: package compiler.lib.verify;
>> 25: 
> 
> You should update the copyright year.

done :)

> 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.

Good question. I think my reasoning was that comparing arbitrary classes requires reflection. And that is rather slow. So by default it would be good if that feature is not enabled, so the user tries to avoid it, and is aware when they enable it explicitly.

But if you think that is not useful, I can remove the feature.

@chhagedorn what do you think?

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

PR Comment: https://git.openjdk.org/jdk/pull/24224#issuecomment-2768381692
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2022262263
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2022265798