On Wed, 18 Oct 2023 01:40:18 GMT, Alex Menkov <[email protected]> wrote:
>> All test cases in getclfld007 had 1 (or 0) field in test classes/interfaces.
>> The change adds several fields in one of the test classes to verify order of
>> the returned fields (as described by GetClassFields spec: "in the order they
>> occur in the class file").
>> Field order in the class file is not guaranteed to be the same as in the
>> source, so information about expected fields and expected order is extracted
>> by ASM (it parses class file sequentially).
>> This allows to drop hardcoded field name/type in native part.
>>
>> Additionally did some test cleanup:
>> - dropped "printdump" stuff (the test always logs reported fields);
>> - removed unused `generic` in native check() method, added deallocation of
>> `name` and `sig`
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> get field order from class file
test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassFields/getclfld007/getclfld007.cpp
line 133:
> 131: }
> 132: }
> 133: }
Nit: I'd suggest to refactor a little bit the content of loop at L111 as below:
for (j = 0; j < fcount; j++) {
if (fields[j] == NULL) {
printf("(%d) fieldID = null\n", j);
result = STATUS_FAILED;
continue; => /you can consider to return here
}
err = jvmti->GetFieldName(clazz, fields[j], &name, &sig, NULL);
if (err != JVMTI_ERROR_NONE) {
printf("(GetFieldName#%d) unexpected error: %s (%d)\n",
j, TranslateError(err), err);
result = STATUS_FAILED; => This result set is missed
continue; => you can consider to return here
}
printf(">>> [%d]: %s, sig = "%s"\n", j, name, sig);
if ((j < field_count) &&
(name == NULL || sig == NULL ||
!equals_str(env, name, fieldArr, j * 2) ||
!equals_str(env, sig, fieldArr, j * 2 + 1))) {
printf("(%d) wrong field: "%s%s"", j, name, sig);
result = STATUS_FAILED;
}
jvmti->Deallocate((unsigned char *)name);
jvmti->Deallocate((unsigned char *)sig);
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16131#discussion_r1364749665
