On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule <hgre...@openjdk.org> wrote:
> Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this
> change adds special casing for `VarHandle.{access-mode}` methods.
>
> The exception message is less exact, but I think that's acceptable.
Great work!
src/hotspot/share/prims/methodHandles.cpp line 1372:
> 1370: */
> 1371: JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject mh, jobjectArray args)) {
> 1372: THROW_MSG_NULL(vmSymbols::java_lang_UnsupportedOperationException(),
> "VarHandle access mode method a cannot be invoked reflectively");
Suggestion:
THROW_MSG_NULL(vmSymbols::java_lang_UnsupportedOperationException(),
"VarHandle access mode methods cannot be invoked reflectively");
Looks like a typo to me.
src/hotspot/share/prims/methodHandles.cpp line 1419:
> 1417: static JNINativeMethod VH_methods[] = {
> 1418: // UnsupportedOperationException throwers
> 1419: {CC "compareAndExchange", CC "([" OBJ ")" OBJ,
> FN_PTR(VH_UOE)},
I recommend ordering these by the order in `AccessMode`, which is also the
declaration order in `VarHandle`; that way, if we add a new access mode, it's
easier for us to maintain this list.
src/hotspot/share/prims/methodHandles.cpp line 1457:
> 1455: JVM_ENTRY(void, JVM_RegisterMethodHandleMethods(JNIEnv *env, jclass
> MHN_class)) {
> 1456: assert(!MethodHandles::enabled(), "must not be enabled");
> 1457: assert(vmClasses::MethodHandle_klass() != nullptr, "should be
> present");
Should we duplicate this assert for `vmClasses::VarHandle_klass()` too?
test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 1:
> 1: /*
The copyright header's year needs an update.
test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 69:
> 67: VarHandle v = handle();
> 68:
> 69: // Try a reflective invoke using a Method, with an array of 0
> arguments
Suggestion:
// Try a reflective invoke using a Method, with the minimal required
argument
test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 72:
> 70:
> 71: Method vhm = VarHandle.class.getMethod(accessMode.methodName(),
> Object[].class);
> 72: Object args = new Object[0];
I recommend naming this `arg`, as this is the single arg to the reflected
method. Had you inlined this, you would have called `vhm.invoke(v, (Object) new
Object[0]);`
-------------
PR Review: https://git.openjdk.org/jdk/pull/20015#pullrequestreview-2157341254
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664744641
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664741601
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664737631
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664753008
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664751627
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664751688
