On Mon, 28 Jul 2025 06:50:47 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> This PR proposes to improve handling of javac's `Flags` in two ways: >> - for each flag, there's now an informational annotation specifying what is >> the target Symbol type. Only targets right now are `TypeSymbol`s, >> `MethodSymbol`s and `VarSymbol`s. If we ran out of flags for `TypeSymbol`s, >> we could split those to module/package/class/type variable, but it does not >> seem to be quite necessary yet. There's an auxiliary special `BLOCK`, which >> is for `JCBlock`. >> - the manually handled `Flags.Flag` enum is replaced with autogenerated >> `FlagsEnum` >> >> This is inspired by: >> https://github.com/openjdk/jdk/pull/26181#pullrequestreview-2997428662 >> >> There may be some better to handle `Flags` eventually, but this hopefully >> improves the current situation at least somewhat, by providing more formal >> way to say the flags' target, and restricting the need to read comments and >> search for free flags. >> >> As a side-effect of this annotation, the >> `test/langtools/tools/javac/flags/FlagsTest.java` now also prints which >> flags are free, for each Symbol type. >> >> (I will remove the `build` label for now, until discussion on javac level is >> done, and will re-add it if we decide the goal to autogenerate the FlagsEnum >> makes sense.) > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Reverting runtime checks, as suggested. make/langtools/tools/flagsgenerator/FlagsGenerator.java line 64: > 62: TypeElement clazz = (TypeElement) trees.getElement(new > TreePath(new TreePath(cut), cut.getTypeDecls().get(0))); > 63: Map<Integer, List<String>> flag2Names = new TreeMap<>(); > 64: Map<FlagTarget, Map<Integer, List<String>>> > target2FlagBit2Fields = new HashMap<>(); Suggestion: Map<FlagTarget, Map<Integer, List<String>>> target2FlagBit2Fields = new EnumMap<>(FlagTarget.class); make/langtools/tools/flagsgenerator/FlagsGenerator.java line 84: > 82: .forEach(target -> > target2FlagBit2Fields.computeIfAbsent(target, _ -> new HashMap<>()) > 83: > .computeIfAbsent(flagBit, _ -> new ArrayList<>()) > 84: > .add(flagName)); Instead of a dedicated loop to verify no overlaps, we can make the nested map `Map<Integer, String>` and fail fast whenever we detect a conflict, like: var oldFlagName = target2FlagBit2Fields.computeIfAbsent(target, _ -> new HashMap<>()).put(flagBit, flagName); if (oldFlagName != null) { // Fail fast code } I personally don't see a reason to collect all conflicting fields for a flag if any of these conflicts already causes a failure. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26452#discussion_r2237874183 PR Review Comment: https://git.openjdk.org/jdk/pull/26452#discussion_r2237871781
