> On Apr 8, 2020, at 5:44 AM, Aleksey Shipilev <sh...@redhat.com> wrote: > > On 4/8/20 2:25 AM, Kim Barrett wrote: >> Webrev: >> https://cr.openjdk.java.net/~kbarrett/8188055/open.04/ > > src/hotspot/share/prims/jvm.cpp: > > *) Do we really need a typedef (L3250) for something that is used once at > L3253? > > 3248 JVM_ENTRY(jboolean, JVM_ReferenceRefersTo(JNIEnv* env, jobject ref, > jobject o)) > 3249 JVMWrapper("JVM_ReferenceRefersTo"); > 3250 typedef HeapAccess<ON_UNKNOWN_OOP_REF | AS_NO_KEEPALIVE> > ReferentAccess; > 3251 const int referent_offset = java_lang_ref_Reference::referent_offset; > 3252 oop ref_oop = JNIHandles::resolve_non_null(ref); > 3253 oop referent = ReferentAccess::oop_load_at(ref_oop, referent_offset); > 3254 return referent == JNIHandles::resolve(o); > 3255 JVM_END
Given a choice between a typedef, an excessively long line (especially without a similarly used-once local constant for referent_offset), or an awkwardly placed line-break, I prefer the first. > *) Double new-line: > > 3256 > 3257 Double new-line separations seem to be the norm in this file. It seems that didn't get followed for the between JVM_ENTRY blocks in this section though. (I forget whether it was me or Per who wrote those, but I did the push, so mea culpa.) > test/hotspot/jtreg/gc/TestReferenceRefersTo.java: > > *) Typo, "unex[p]ected": > > 134 fail(which + " refers to unexected value”); Fixed. > Otherwise seems fine. Thanks. Waiting for other discussion to resolve before sending out any new webrevs.
