On Tue, 24 Mar 2026 07:08:18 GMT, Stefan Karlsson <[email protected]> wrote:
>> David Holmes has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address Stefan's concern about THREAD vs. CHECK
>
> Looks good to me. Some thoughts that popped up when reading this change:
Thanks for the reviews @stefank and @fparain !
> src/hotspot/share/prims/jni.cpp line 2395:
>
>> 2393: assert(a->klass()->is_refined_objArray_klass(), "must be");
>> 2394: if (v == nullptr ||
>> v->is_a(ObjArrayKlass::cast(a->klass())->element_klass())) {
>> 2395: a->obj_at_put(index, v, THREAD);
>
> Some musing, but not entirely necessary for the review: I'm not entirely sure
> that changing `CHECK` to `THREAD` is beneficial for readers of the code. With
> `CHECK` I understood that the code returned at that point if an exception is
> thrown. With `THREAD` I know have to read to the end of the function to see
> that there's no other code that is being run when we hit an exception. OTOH,
> that's probably a prudent thing to do for both cases given how this code is
> if/else-branchy instead of using "early returns".
I added the early return to make it clear. I have a mini-mission to remove
unnecessary CHECK usage where I see it.
-------------
PR Comment: https://git.openjdk.org/valhalla/pull/2254#issuecomment-4121921245
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2254#discussion_r2984764439
