On Wed, 25 Oct 2023 09:46:44 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> src/hotspot/share/runtime/jniHandles.cpp line 202:
>>
>>> 200: ShouldNotReachHere();
>>> 201: }
>>> 202: } else if (is_local_handle(thread, handle) ||
>>> is_frame_handle(thread, handle)) {
>>
>> Should we still add `ShouldNotReachHere()` at global `else` branch? This
>> would make the if-else chain exhaustive with the early warning if some
>> handle type is not used. The prior code did this already.
>
> Not sure what you're saying here. As far as I understand the intent of this
> code is to check whether the handle is of a certain type, and if it's not
> recognized, return `JNIInvalidRefType`. So, I'm not sure there should be any
> `ShouldNotReachHere()` in this code.
>
> We can add an `else` branch that returns `JNIInvalidRefType` though.
The old code would throw `ShouldNotReachHere()` if we did not recognize the
handle type:
https://github.com/openjdk/jdk/blob/d2d1592dd94e897fae6fc4098e43b4fffb6d6750/src/hotspot/share/runtime/jniHandles.cpp#L207
I think the new code should still keep it, like so:
} else if (is_local_handle(thread, handle) || is_frame_handle(thread, handle))
{
...
} else {
ShouldNotReachHere();
}
That way, if we ever have another handle type, we would hit the `else` branch
instead of silently returning `JNIInvalidRefType`. That is, our condition chain
would still be exhaustive, catching unexpected values explicitly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16349#discussion_r1371481515
