[lworld] RFR: 8281463: [lworld] VALUE / PRIMITIVE modifiers should be supported by reflection [v3]

[Roger Riggs]

On Tue, 7 Jun 2022 18:09:52 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Roger Riggs 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 four additional commits since the last revision:
>> 
>>  - Merge branch 'lworld' into 8281463-value-modifiers
>>  - Add Class.is(AccessFlag) to test for a single flag.
>>  - Restore AccessFlag SUPER
>>  - 8281463: [lworld] VALUE / PRIMITIVE modifiers should be supported by reflection
>>    Use java.lang.reflect.AccessFlag for equivalent functions for value objects.
>>    Corrected ModuleDescriptor.STATIC flags to be STATIC_PHASE
>>    Added flags to j.l.reflect.Modifier.
>
> src/java.base/share/classes/java/lang/Class.java line 1511:
> 
>> 1509:      * @param flag an {@link AccessFlag}
>> 1510:      */
>> 1511:     public boolean is(AccessFlag flag) {
> 
> Is this API needed?   We can do `Class.accessFlags().contains(flag)`.   This discussion should belong to PR 7445 (see my comment in the `Modifier`).

There are significant performance concerns. The current implementation of getModifers() is an intrinsic and all of the tests for modifiers are boolean operations against static constants.
Doing a Set.contains against a set that is re-built every time getAccessFlags() is called is a non-starter.
Many of the access flags have dedicated methods in j.l.Class, for example, isPublic, isStatic, etc.
It would improve usability and performance to be able to check for an accessFlag in a simple direct operation.
I'll keep this for the time being.

> src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 125:
> 
>> 123:     /**
>> 124:      * The access flag {@code ACC_IDENTITY} corresponding to the
>> 125:      * source modifier {@link Modifier#VALUE value} with a mask
> 
> typo: `{@link Modifier#IDENTITY identity}`

The new Valhalla specific access flags are not define in Modifier. So a link is not warranted.
The reference to the JLS section would be appropriate.

> src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 134:
> 
>> 132:      * Now the flag has been repurposed as ACC_IDENTITY.
>> 133:      */
>> 134: //    SUPER(0x0000_0020, false, Set.of(Location.CLASS)),
> 
> I expect the test should be updated instead of commenting this out.

By the time this is finalized, there will be no ACC_SUPER in the JVMs spec.

> src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 139:
> 
>> 137:      * The access flag {@code ACC_IDENTITY} with a mask value of {@code 0x0020}.
>> 138:      */
>> 139:     IDENTITY(0x0000_0020, false, Set.of(Location.CLASS)),
> 
> It can also be in `Location.INNER_CLASS`.   Same for value and primitive.

fixed

> src/java.base/share/classes/java/lang/reflect/Modifier.java line 46:
> 
>> 44:  * The {@link AccessFlag} class provides a model that distinguishes between access flags
>> 45:  * for classes, methods, and field. The {@link Class#accessFlags()} and {@link Member#accessFlags()}
>> 46:  * methods provide the access flags for the respective class, method, or field.
> 
> The discussion of this proposed spec change belongs in [PR 7445](https://github.com/openjdk/jdk/pull/7445).    I would prefer to keep the version in Valhalla sync'ed with PR 7445 except the spec change to support the new `identity`, `value`, `primitive` modifiers.   Other changes not relevant to Valhalla should be done in the PR target to the main line.

We'll come back to it when the PR is finally reviewed.
We can resolve any differences when Valhalla is to be integrated.

> src/java.base/share/classes/jdk/internal/org/objectweb/asm/Type.java line 686:
> 
>> 684: 
>> 685:     static boolean isPrimitiveClass(Class<?> clazz) {
>> 686:         return clazz.is(AccessFlag.PRIMITIVE);
> 
> Suggestion:
> 
>         return clazz.accessFlags().contains(AccessFlag.PRIMITIVE);

Not performant.

> test/langtools/tools/javac/valhalla/lworld-values/InnerClassAttributeValuenessTest.java line 55:
> 
>> 53:         System.out.println("accessFlags: " + flags);
>> 54: 
>> 55:         if (!Inner.class.is(AccessFlag.VALUE))
> 
> Suggestion:
> 
>         if (!flags.contains(AccessFlag.VALUE))

No, not performant.

> test/langtools/tools/javac/valhalla/lworld-values/InnerClassAttributeValuenessTest.java line 57:
> 
>> 55:         if (!Inner.class.is(AccessFlag.VALUE))
>> 56:             throw new AssertionError("Value flag missing");
>> 57:         if (!Inner.class.is(AccessFlag.PRIMITIVE))
> 
> Suggestion:
> 
>         if (!flags.contains(AccessFlag.PRIMITIVE))

Ditto

-------------

PR: https://git.openjdk.java.net/valhalla/pull/698