On Wed, 7 Jun 2023 16:06:54 GMT, Mandy Chung <[email protected]> wrote:
> `FieldSetAccessibleTest` attempts to load all JDK classes and test
> `setAccessible` on their public fields to work properly. It should filter
> the modules that directly and indirectly depend on `jdk.internal.vm.compiler`
> and `jdk.internal.vm.compiler.management` as they are upgradeable and also
> provide APIs to add qualifed exports dynamically.
LGTM. I have suggested adding a comment to explain what's going on in the code
added.
test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java line 323:
> 321: mods.stream()
> 322: .flatMap(mn ->
> findDeps(mn, inverseDeps).stream()))
> 323: .collect(Collectors.toSet());
Suggestion:
// Build a list of modules that should be filtered out. These are
the deploy modules,
// plus all modules that directly or indirectly require
jdk.internal.vm.compiler and
// jdk.internal.vm.compiler.management, as these are upgradeable
and also provide
// APIs to add qualified exports dynamically.
Set<String> filters = Stream.concat(deployModules.stream(),
mods.stream()
.flatMap(mn -> findDeps(mn,
inverseDeps).stream()))
.collect(Collectors.toSet());
-------------
Marked as reviewed by dfuchs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14362#pullrequestreview-1468243596
PR Review Comment: https://git.openjdk.org/jdk/pull/14362#discussion_r1221957008
