On Tue, 10 Mar 2026 18:38:33 GMT, Chen Liang <[email protected]> wrote:
>> Currently, the preview access flag enum constants are made available with a >> ClassFileFormatVersion.CURRENT_PREVIEW_FEATURES constant. Such a constant is >> questionable design. >> >> We can have an approach to model preview access flags without such a >> constant, losing the ability for users to inspect through >> `ClassFileFormatVersion`, but otherwise keeping these interfaces intact: >> >> 1. `accessFlags()` factory methods in core reflection >> 2. Inspection of AccessFlag through ClassFile API >> 3. The outputs of javap >> >> The new design makes use of a new internal API, >> `jdk.internal.reflect.PreviewAccessFlags`. It replaces the AccessFlag APIs >> that previously took `ClassFileFormatVersion.CURRENT_PREVIEW_FEATURES`. This >> introduces a new qualified export from `java.base/jdk.internal.reflect` to >> `jdk.jdeps` for Javap usage. >> >> In addition, with the recent awareness that core reflection/HotSpot only has >> one unique representation of modifiers, we can remove some contrived >> representation of access flags present in mainline (that required pulling >> class version from Class mirrors) and migrate to our simplified system that >> decodes the uniform representation used by hotspot. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Dead import > - Merge branch 'lworld' of https://github.com/openjdk/valhalla into > fix/lw-remove-cffv-preview > - Bugs and redundant diffs > - Silly test > - Remove PREVIEW class file format version and rewire stuff The overall encapsulation of interpreting flag/modifier bits in preview class files and --enable-preview is good. src/java.base/share/classes/jdk/internal/reflect/PreviewAccessFlags.java line 36: > 34: import static java.lang.reflect.AccessFlag.*; > 35: > 36: /// Provides access to preview Access Flag information. Elaborate a bit more on the purpose. Describe interpreting the flag bits for class preview class files. And is called instead of the AccessFlag.maskToAccessFlags() when --enable-preview. src/java.base/share/classes/jdk/internal/reflect/PreviewAccessFlags.java line 61: > 59: /// Parses access flag for preview class files. > 60: /// @throws IllegalArgumentException if there is unrecognized flag bit > 61: public static Set<AccessFlag> parse(int flags, Location location) { `parse` isn't a great name for this function. `maskToAccessFlags` is used elsewhere. src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 455: > 453: /// Parses a runtime modifier from core reflection/Hotspot into > access flags. > 454: /// Note that there is one unique representation in hotspot, so we > don't need version argument. > 455: /// Technically this should never through IAE. typo: "through" -> "throw" src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 472: > 470: } > 471: } catch (IllegalArgumentException e) { > 472: throw new InternalError("Inconsistent hotspot > representation", e); The word "modifiers" would make this internal error message more useful. Or drop the message and rely on the line number. (avoid wasting space on an unuseful string). src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 479: > 477: return Collections.unmodifiableSet(buf); // Preserve > iteration order > 478: } > 479: return ans; Suggestion: try { Set<AccessFlag> ans = (PreviewFeatures.isEnabled()) ? PreviewAccessFlags.parse(modifiers, location) : AccessFlag.maskToAccessFlags(modifiers, location); if (injectStrict) { Set<AccessFlag> buf = EnumSet.copyOf(ans); buf.add(AccessFlag.STRICT); ans = Collections.unmodifiableSet(buf); // Preserve iteration order } return ans; } catch (IllegalArgumentException e) { throw new InternalError("Inconsistent hotspot representation", e); } The method name `parse` isn't a great name given the function, `maskToAccessFlags` is clearer. If this function is called often and STRICT is usually set, it will cause a bit more gc. The immutability of the intermediate results makes it more expensive. ------------- PR Review: https://git.openjdk.org/valhalla/pull/2209#pullrequestreview-3930528004 PR Review Comment: https://git.openjdk.org/valhalla/pull/2209#discussion_r2919411940 PR Review Comment: https://git.openjdk.org/valhalla/pull/2209#discussion_r2919393924 PR Review Comment: https://git.openjdk.org/valhalla/pull/2209#discussion_r2919202262 PR Review Comment: https://git.openjdk.org/valhalla/pull/2209#discussion_r2919229321 PR Review Comment: https://git.openjdk.org/valhalla/pull/2209#discussion_r2919358533
