On Tue, 21 Jun 2022 22:30:43 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 incrementally with one additional > commit since the last revision: > > Remove implSpec tag from Executable.accessFlags since the class is sealed. This version looks good. It'd be good to extend the tests to include test cases for `Location.INNER_CLASS`. src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 290: > 288: // Intentionally using Set rather than EnumSet since EnumSet is > 289: // mutable. > 290: private Set<Location> locations; these instance fields can be made final. test/jdk/java/lang/reflect/AccessFlag/BasicAccessFlagTest.java line 47: > 45: } > 46: > 47: private static void testSourceModifiers() throws Exception { A comment about what this verifies would be helpful, like "for AccessFlag that is a source modifier, a Modifier constant is defined." test/jdk/java/lang/reflect/AccessFlag/ClassAccessFlagTest.java line 27: > 25: * @test > 26: * @bug 8266670 > 27: * @summary Test expected AccessFlag's on fields. typo? s/fields/classes/ test/jdk/java/lang/reflect/AccessFlag/ClassAccessFlagTest.java line 159: > 157: } > 158: > 159: // Is there is at least one special enum constant, the enum class typo s/Is/If ------------- Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.org/jdk/pull/7445
