> On Jul 20, 2020, at 1:53 AM, David Holmes <[email protected]> wrote: > > Hi Kim, > > Thanks for looking at this. > > Updated webrev at: > > http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/
Looks good. > > On 20/07/2020 3:22 pm, Kim Barrett wrote: >>> On Jul 20, 2020, at 12:16 AM, David Holmes <[email protected]> wrote: >> src/hotspot/share/prims/jni.cpp >> 743 result = JNIHandles::make_local(THREAD, result_handle()); >> jni_PopLocalFrame is now using a mix of "thread" and "THREAD", where >> previously it just used "thread". Maybe this change shouldn't be made? >> Or can the other uses be changed to THREAD for consistency? > > "thread" and "THREAD" are interchangeable for anything expecting a "Thread*" > (and somewhat surprisingly a number of API's that only work for JavaThreads > actually take a Thread*. :( ). I had choice between trying to be file-wide > consistent with the make_local calls, versus local-code consistent, and used > THREAD as it is available in both JNI_ENTRY and via TRAPS. But I can > certainly make a local change to "thread" for local consistency. I don’t feel strongly either way. It just struck me as a little odd to have the mix in close proximity, especially since I think consistently using either one might work in this function. But being consistent about make_local usage has something to be said for it too. >> src/hotspot/share/prims/jvm.cpp >> The calls to JvmtiExport::post_vm_object_alloc have to use "thread" >> instead of "THREAD", even though other places nearby are using >> "THREAD". That inconsistency is kind of unfortunate, but doesn't seem >> easily avoidable. > > Everything that uses THREAD in a JVM_ENTRY method can be changed to use > "thread" instead. But I'm not sure it's a consistency worth pursuing at least > as part of these changes (there are likely similar issues with most of the > touched files). Yeah, it’s not really obvious whether to use THREAD or thread in some cases. But I agree that addressing any inconsistencies there is mostly out of scope for this change.
