On Wed, 11 Mar 2026 15:58:11 GMT, Roger Riggs <[email protected]> wrote:
>> 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 > > 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. Renamed the method to `maskToAccessFlags`. I prefer keeping the try-catch as small as possible to avoid accidental catches. I investigated STRICT a bit more and think this is likely a hotspot runtime issue. We can remove this workaround if hotspot uniformly drops STRICT as an unrecognized modifier. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2209#discussion_r2920993998
