RFR: JDK-8266670: Better modeling of access flags in core reflection [v33]

[Mandy Chung]

On Tue, 21 Jun 2022 22:30:43 GMT, Joe Darcy <darcy at 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