On Tue, 24 Oct 2023 00:46:54 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> FilteredFieldStream is intended to filter out some fields which does not
> represent valid java objects.
> Currently the only filtered field is "constantPoolOop" from
> jdk.internal.reflect.ConstantPool class.
> The change fixes FilteredFieldStream implementation to handle cases when
> filtered fields is the last field of the class ("constantPoolOop" is the only
> field of jdk.internal.reflect.ConstantPool)
>
> Testing:
> - new test added that compares results of GetClassFields JVMTI function (it
> uses FilteredFieldStream) with Class.getDeclaredFields();
> - test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassFields tests
The fix itself looks good. I've posted a couple of nits on the new test.
Thanks,
Serguei
test/hotspot/jtreg/serviceability/FilteredFields/FilteredFieldsTest.java line
25:
> 23:
> 24: /*
> 25: * @test
@bug and @summary is needed as well.
The test folder has to be at least `test/hotspot/jtreg/serviceability/jvmti`.
It would be nice to check with Leonid on this.
test/hotspot/jtreg/serviceability/FilteredFields/libFilteredFieldsTest.cpp line
63:
> 61: jfieldID *fields = nullptr;
> 62:
> 63: jvmtiError err = jvmti->GetClassFields(clazz, &fcount, &fields);
Nit: The function `check_jvmti_status()` or `check_jvmti_error()` from
jvmti_common.h` can be used here.
The function `fatal()` in other cases like at line 54 can be used.
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16328#pullrequestreview-1702664188
PR Review Comment: https://git.openjdk.org/jdk/pull/16328#discussion_r1375135273
PR Review Comment: https://git.openjdk.org/jdk/pull/16328#discussion_r1375137583
