On Apr 8, 2020, at 5:44 AM, Aleksey Shipilev <shade@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.