On Thu, 1 Feb 2024 00:17:57 GMT, Chris Plummer wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 93:
>
>> 91:
>> 92: /*
>> 93: Per jvmtiHeapReferenceInfoField spec:
>
> I think you should also include mention that this is for
> JVMTI_HEAP_REFERENCE_STATIC_FIELD.
>
> Indentation should be fixed to match our C++ comment style, and the "bullets"
> from the spec should be included to make it clearer that these are two lists
> of steps.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 108:
>
>> 106: where n is the count of the fields in all the superinterfaces of I.
>> 107:
>> 108: 'Klass' struct contains all required data to calculate field indeces.
>
> "indices"
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 153:
>
>> 151:
>> 152: /*
>> 153: For each test object 'Object' struct is created and pointer to it set
>> as a tag for jobject.
>
> Suggestion:
>
> For each test object, the 'Object' struct is created and a pointer to it is
> set as the jobject's tag.
Fixed.
Updated comment for 'Klass' class the same way
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 225:
>
>> 223:
>> 224: // Explores all interfaces implemented by 'klass' sorting out duplicates
>> 225: // and store them in the 'arr' starting from 'index'.
>
> Suggestion:
>
> // Explores all interfaces implemented by 'klass', sorts out duplicates,
> // and stores the interfaces in the 'arr' starting from 'index'.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 254:
>
>> 252:
>> 253: // And explore its superinterfaces.
>> 254: count += fill_interfaces(arr, index + count, env,
>> interfaces[i]);
>
> I assume here we are always dealing with test classes that are known not to
> be deeply nested.
Yes, we explore only test classes/interfaces
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 348:
>
>> 346:
>> 347: jclass obj_klass = env->GetObjectClass(obj);
>> 348:
>
> Unnecessary newline.
removed.
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 350:
>
>> 348:
>> 349: Klass* klass = Klass::explore(env, obj_klass);
>> 350:
>
> Unnecessary newline.
removed
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 492:
>
>> 490: Java_FieldIndicesTest_prepare(JNIEnv *env, jclass cls, jobject testObj)
>> {
>> 491: Object::explore(env, testObj);
>> 492: fflush(0);
>
> stdout?
fflush(0) flashes all opened streams (including stout and stderr).
It's to handle the case when some shared test routines log to stderr.
But flash() expects `FILE *`, so I updated all calls to use `nullptr`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473693579
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473695968
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473696254
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473696356
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473698778
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701344
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701389
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1473701268
