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/7ac698ba...14980605 I took a closer look at the proposed APIs. I think it's in a good state to target for 19. I skimmed on the existing JDK usage of `getModifiers` other than a trivial test e.g. is public, final, etc and the proposed API works well (btw no plan to convert them and just use those cases for validation). The value of `ACC_SUPER` and `ACC_STRICT` could possibly be reused for new access flags. It may be useful to document when the flag becomes obsolete. Nit: the enum constants are listed in the order of the mask value, which I like. Some enum constants reference the `Modifer` constants but I think it'd help to see the mask value here consistently for all entries. One go-to place in the source if I want to find the value of different flags. src/java.base/share/classes/java/lang/Class.java line 1328: > 1326: * @since 19 > 1327: */ > 1328: public Set<AccessFlag> accessFlags() { The access flags of a hidden class has no difference than that of a normal class. A hidden class is created with normal `ClassFile` except that it's created via `Lookup::defineHiddenClass` API. I think the `Class::accessFlags` method should specify the access flags for primitive type, void, and array classes as `Class::getModifiers` specified. src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 216: > 214: > 215: /** > 216: * {@return an unmodifiable set of the module {@linkplain > AccessFlag Suggestion: * {@return a possibly-empty unmodifiable set of the module {@linkplain AccessFlag as specified in the `@return` of the `modifiers()` method. Same comment applies to the `accessFlags` method in `ModuleDescriptor.Opens` and other classes. src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 44: > 42: * <em>not</em> have an access flag, such as {@code sealed} (JVMS > 43: * {@jvms 4.7.31}) and some access flags have no corresponding > 44: * modifier, such as {@linkplain SYNTHETIC synthetic}. Suggestion: * modifier, such as {@linkplain #SYNTHETIC synthetic}. src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 186: > 184: /** > 185: * The access flag {@code ACC_ABSTRACT}, corresponding to the > 186: * source modifier {@code link Modifier#ABSTRACT abstract}. Suggestion: * source modifier {@link Modifier#ABSTRACT abstract}. src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 306: > 304: * Note that since these locations represent class file structures > 305: * rather than language structures many language structures, such > 306: * as constructors and interfaces, are <em>not</em> present. missing `@since 19` ------------- PR: https://git.openjdk.java.net/jdk/pull/7445
