On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy <da...@openjdk.org> wrote:
>> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy 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 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/0053820b...14980605 Here, I argue for the case of splitting different types of access flags into distinct enums, such as `MemberAccessFlag`, `ModuleRequiresAccessFlag`, `ModuleExportsAccessFlag` implementing a sealed interface `AccessFlag` that keeps all its existing methods, since most of those access flags will never be returned in the same collection. The `accessFlags()` method should return a `Set<? extends AccessFlag>`, which is 1. safe as the set is read-only; 2. can be overridden with `Set<? extends MemberAccessFlag>` (For forward compability, we can make `MemberAccessFlag` an interface with static final fields, implemented by some package-private enum in case we want to split it into more specific types down the road). But you may ask, what about `SYNTHETIC`, `MANDATED`, `FINAL`? Aren't they shared by almost all of the locations? What if users test with incorrect enum instance, such as looking for the `MemberAccessFlag.SYNTHETIC` in `moduleDescriptor.accessFlags()`? This problem can be prevented: 1. by making `ModuleDescriptor.accessFlags()` return `Set<? extends ModuleDescriptorAccessFlag>`, thus the IDE can easily detect misuse when they insert the access flag of a different type; 2. In the current hodgepodge `AccessFlag`, we have `STATIC` and `STATIC_PHASE`, and the incorrect `ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC)` call is much more subtle, especially to new users of this class. Arguably, this misuse would be way worse than that in the distinct enum case. ------------- PR: https://git.openjdk.java.net/jdk/pull/7445
