On Fri, 13 Mar 2026 23:41:19 GMT, Chen Liang <[email protected]> wrote:
>> Currently, the access flags in core reflection are parsed through a central >> factory that dispatches through ReflectionFactory, which examines the >> location plus the class file format version, so that if a Method has STRICT, >> we call AccessFlag.maskToAccessFlag with the right class file format version >> so we do not fail on encountering STRICT. >> >> After recent studies in project Valhalla, we noticed that Hotspot has a >> uniform representation of access flags for all class file versions. This >> means that if we can avoid passing the ClassFileVersion complexities and >> just parse based on the uniform representation Hotspot recognizes. >> >> This change requires moving the AccessFlagSet to jdk.internal so it could be >> accessed by parsers. But we can remove the JavaLangAccess backdoor to fetch >> the class file versions. >> >> Also see the companion Valhalla PR: >> https://github.com/openjdk/valhalla/pull/2209 > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Copyright years Reviewed PR 30248: 8380076: Refactor accessFlags() parsing in core reflection. The refactoring is correct — `Class.accessFlags()` condition equivalence verified, `AccessFlagSet.contains()` handles shared-mask flags correctly, performance improved (eliminated SharedSecrets call and class file version resolution from hot path). All 23 existing tests pass. One suggestion: `AccessFlagSet` would benefit from dedicated unit tests for its bit-manipulation internals (see inline comment). No issues found. LGTM! ✅ *Reviewed by glm-5.1 via Qwen Code /review* src/java.base/share/classes/jdk/internal/reflect/AccessFlagSet.java line 58: > 56: /// If a bit position does not have an access flag defined, its > corresponding > 57: /// array entry is `null`. > 58: public final class AccessFlagSet extends AbstractSet<AccessFlag> { **[Suggestion]** The new `AccessFlagSet` class has non-trivial bit-manipulation logic (shared-mask value disambiguation for flags like SUPER/OPEN/TRANSITIVE at 0x0020, immutable Set contract with UOE on mutations, iterator from LSB to MSB) but no dedicated unit test. Existing `AccessFlag` tests exercise it indirectly through the public `accessFlags()` and `flags()` APIs, but don't directly verify `AccessFlagSet` internals: iteration order, `contains()` edge cases with aliased bits, `isEmpty()`/`size()`, or the immutable set contract (UOE on `add`/`remove`/`clear`). Consider adding a dedicated jtreg test (using `--add-opens` to access `jdk.internal.reflect.AccessFlagSet`, or testing through the public API with targeted assertions). _— glm-5.1 via Qwen Code /review_ ------------- Marked as reviewed by swen (Committer). PR Review: https://git.openjdk.org/jdk/pull/30248#pullrequestreview-4066922430 PR Review Comment: https://git.openjdk.org/jdk/pull/30248#discussion_r3043934880
