Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 1 Jun 2022 05:09:42 GMT, ExE Boss wrote: >> Or `AbstractMethodError`, which is what `Executable::getParameterCount()` >> does: >> https://github.com/openjdk/jdk/blob/e751b7b1b6f7269a1fe20c07748c726536388f6d/src/java.base/share/classes/java/lang/reflect/Executable.java#L248-L258 > > Actually, it should probably be `UnsupportedOperationException` instead. Hmm. If we had sealed classes and interfaces back in JDK 1.1 when Member was added, it most likely would have been added as sealed interface. But, we didn't have sealed interfaces back then so Member can (potentially) be extended by anyone. IIRC, there are a few classes implementing Member outside of the JDK. So, when adding a new method would JDK 20, the method should certainly have be default method. I think throwing UnsupportedOperationException in the default is marginally better than the alternatives. I'll update the PR accordingly in a subsequent push. Thanks. - PR: https://git.openjdk.org/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Tue, 31 May 2022 17:20:08 GMT, Rémi Forax wrote:
>> 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 32 additional commits
>> since the last revision:
>>
>> - Target JDK 20 rather than 19.
>> - Merge branch 'master' into JDK-8266670
>> - Add mask values to constants' javadoc.
>> - Implement review feedback from mlchung.
>> - Fix type in @throws tag.
>> - Merge branch 'master' into JDK-8266670
>> - Respond to review feedback.
>> - Merge branch 'master' into JDK-8266670
>> - Make workding changes suggested in review feedback.
>> - Merge branch 'master' into JDK-8266670
>> - ... and 22 more: https://git.openjdk.org/jdk/compare/3d668090...05cf2d8b
>
> src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 130:
>
>> 128: MANDATED(AccessFlag.MANDATED.mask());
>> 129:
>> 130: private int mask;
>
> it should be declared final ?
Sure; will change.
> src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 134:
>
>> 132: this.mask = mask;
>> 133: }
>> 134: private int mask() {return mask;}
>
> seem useless, `mask` can be accessed directly
Hmm. Stylistically, I prefer methods.
-
PR: https://git.openjdk.org/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since
> the last revision:
>
> - Target JDK 20 rather than 19.
> - Merge branch 'master' into JDK-8266670
> - Add mask values to constants' javadoc.
> - Implement review feedback from mlchung.
> - Fix type in @throws tag.
> - Merge branch 'master' into JDK-8266670
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - ... and 22 more:
> https://git.openjdk.java.net/jdk/compare/1b91dbd2...05cf2d8b
Testing a mask for one AccessFlag looks like:
if ((modifiers & AccessFlag.STRICTFP.mask()) != 0) { ... }
A method on AccessFlag to test if its mask value is present in a 32-bit value
would/could make the code easier to read and write.
if (AccessFlag.STRICTFP.isPresent(modifiers)) { ...}
The method name should be chosen to make the test clear and readable.
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/187b34b0...05cf2d8b Marked as reviewed by asotona (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 1 Jun 2022 05:01:33 GMT, ExE Boss wrote:
>> src/java.base/share/classes/java/lang/reflect/Member.java line 96:
>>
>>> 94: */
>>> 95: public default Set accessFlags() {
>>> 96: return Set.of();
>>
>> Is is not better to throw a NoSuchMethodError instead of Set.of() if there
>> is no implementation.
>
> Or `AbstractMethodError`, which is what `Executable::getParameterCount()`
> does:
> https://github.com/openjdk/jdk/blob/e751b7b1b6f7269a1fe20c07748c726536388f6d/src/java.base/share/classes/java/lang/reflect/Executable.java#L248-L258
Actually, it should probably be `UnsupportedOperationException` instead.
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Tue, 31 May 2022 17:26:35 GMT, Rémi Forax wrote:
>> 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 32 additional commits
>> since the last revision:
>>
>> - Target JDK 20 rather than 19.
>> - Merge branch 'master' into JDK-8266670
>> - Add mask values to constants' javadoc.
>> - Implement review feedback from mlchung.
>> - Fix type in @throws tag.
>> - Merge branch 'master' into JDK-8266670
>> - Respond to review feedback.
>> - Merge branch 'master' into JDK-8266670
>> - Make workding changes suggested in review feedback.
>> - Merge branch 'master' into JDK-8266670
>> - ... and 22 more:
>> https://git.openjdk.java.net/jdk/compare/db2b501c...05cf2d8b
>
> src/java.base/share/classes/java/lang/reflect/Member.java line 96:
>
>> 94: */
>> 95: public default Set accessFlags() {
>> 96: return Set.of();
>
> Is is not better to throw a NoSuchMethodError instead of Set.of() if there is
> no implementation.
Or `AbstractMethodError`, which is what `Executable::getParameterCount()` does:
https://github.com/openjdk/jdk/blob/e751b7b1b6f7269a1fe20c07748c726536388f6d/src/java.base/share/classes/java/lang/reflect/Executable.java#L248-L258
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since
> the last revision:
>
> - Target JDK 20 rather than 19.
> - Merge branch 'master' into JDK-8266670
> - Add mask values to constants' javadoc.
> - Implement review feedback from mlchung.
> - Fix type in @throws tag.
> - Merge branch 'master' into JDK-8266670
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - ... and 22 more:
> https://git.openjdk.java.net/jdk/compare/74cd2687...05cf2d8b
src/java.base/share/classes/java/lang/reflect/Member.java line 96:
> 94: */
> 95: public default Set accessFlags() {
> 96: return Set.of();
Is is not better to throw a NoSuchMethodError instead of Set.of() if there is
no implementation.
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since
> the last revision:
>
> - Target JDK 20 rather than 19.
> - Merge branch 'master' into JDK-8266670
> - Add mask values to constants' javadoc.
> - Implement review feedback from mlchung.
> - Fix type in @throws tag.
> - Merge branch 'master' into JDK-8266670
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - ... and 22 more:
> https://git.openjdk.java.net/jdk/compare/1545e825...05cf2d8b
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 130:
> 128: MANDATED(AccessFlag.MANDATED.mask());
> 129:
> 130: private int mask;
it should be declared final ?
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 134:
> 132: this.mask = mask;
> 133: }
> 134: private int mask() {return mask;}
seem useless, `mask` can be accessed directly
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/3d558a34...05cf2d8b I think `Modifier` API is as good as it is right now. The question is whether we keep `Modifier` API as is for the existing usage or enhancing it for the new modifiers such as `value` and `primitive`. No benefit of replacing existing uses of `Modifier` to the new `AccessFlag` API as most existing use are simple tests to determine if it's public, static, final, non-public etc. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Fri, 27 May 2022 20:21:12 GMT, Mandy Chung wrote: > With the `AccessFlag` API, what is the role of the `Modifier` API going > forward? [Value Objects JEP](https://openjdk.java.net/jeps/8277163) defines > the new `identity` and `value` modifiers. [PR > #698](https://github.com/openjdk/valhalla/pull/698) proposes to add > `Modifier.IDENTITY` and `Modifier.VALUE` constants as the `identity` and > `value` modifiers are encoded in a class file using the `ACC_IDENTITY` and > `ACC_VALUE` flags. However, with the new improved `AccessFlag` API, the new > flags will be defined in the `AccessFlag` API. I think we should avoid adding > the new flags in `Modifier` and leave it for the existing usage. Use > `AccessFlag` for new features. Looking over the existing uses of Modifier, as relates to Class.getModifiers(), there are simple uses testing for an individual access flag and others that expect a full set of modifier bits that apply to the class. There are a few places that need all the bits to compose a new set of modifier bits and a few others that test for combinations of bits. `Class.getModifiers()` is intrinsified, testing against various bit patterns using the static methods of `Modifier` is very performant. The `Modifer.toString()` method cannot distinguish the flags overloaded by the Class vs Member cases. Modifier.toString() is used by both Class and Member `toString` methods. Methods to convert a Set to a mask and to produce the 'toString' of the set would be useful. To replace all uses of Modifier the existing use cases will need some attention. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Fri, 27 May 2022 21:53:38 GMT, Roger Riggs wrote: > As for retaining SUPER, both SUPER and IDENTITY have location class and > according to the BasicAccessFlagTest, the bit patterns should be disjoint If the defined set of class locations, considered over time, have non-disjoint definitions, then the test would need to be modified accordingly of course. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/f2d5c3d7...05cf2d8b As for retaining SUPER, both SUPER and IDENTITY have location class and according to the BasicAccessFlagTest, the bit patterns should be disjoint - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/fb4068e0...05cf2d8b Please comment here: https://github.com/openjdk/valhalla/pull/698 The AccessFlag API identifies Modifier as *the* primary definition of source elements. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/43363358...05cf2d8b With the `AccessFlag` API, what is the role of the `Modifier` API going forward? [Value Objects JEP](https://openjdk.java.net/jeps/8277163) defines the new `identity` and `value` modifiers. [PR #698](https://github.com/openjdk/valhalla/pull/698) proposes to add `Modifier.IDENTITY` and `Modifier.VALUE` constants as the `identity` and `value` modifiers are encoded in a class file using the `ACC_IDENTITY` and `ACC_VALUE` flags. However, with the new improved `AccessFlag` API, the new flags will be defined in the `AccessFlag` API. I think we should avoid adding the new flags in `Modifier` and leave it for the existing usage. Use `AccessFlag` for new features. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/2ff12e53...05cf2d8b I have the same view as Joe. `ACC_SUPER` has been specified in JVMS and set in class files of an older version. If `ACC_SUPER` would be removed from JVMS before this API becomes final, the `AccessFlag` API would be revised and not to define it at that time. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/75552cfd...05cf2d8b To reinforce the argument not to propagate ACC_SUPER to the new API. It is NOT currently defined in j.l.reflect.Modifier, so there are no existing dependencies on a symbol. Class.getModifiers() *never* returns ACC_SUPER; the VM explicitly removes it (since 2007, for all class file versions). The note in the JVMS spec indicates it is retained only for source compatibility with earlier releases. The JVMS spec considers the flag to always be set. and It is always unset in the modifiers returned from getModifiers(). Defining the value where it has not been defined before has no useful purpose. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 19:22:51 GMT, Roger Riggs wrote: > AccessFlags.SUPER can/should be removed; it is unused and will be redefined > in the [Value Objects JEP](https://openjdk.java.net/jeps/8277163). It will be > a cleaner transition if there is no opportunity to create a dependency on the > obsolete flag. Hmm. I don't agree with that conclusion. I see the role of AccessFlags is to model what is in the class file. ACC_SUPER has been documented and in multiple versions of the JVMS and has appeared set in class files. If the bit position is reused, that is fine and the API can accommodate that overlap, unlike a bit-flag based model. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/65e59633...05cf2d8b AccessFlags.SUPER can/should be removed; it is unused and will be redefined in the [Value Objects JEP](https://openjdk.java.net/jeps/8277163). It will be a cleaner transition if there is no opportunity to create a dependency on the obsolete flag. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
> 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 32 additional commits since the last revision: - Target JDK 20 rather than 19. - Merge branch 'master' into JDK-8266670 - Add mask values to constants' javadoc. - Implement review feedback from mlchung. - Fix type in @throws tag. - Merge branch 'master' into JDK-8266670 - Respond to review feedback. - Merge branch 'master' into JDK-8266670 - Make workding changes suggested in review feedback. - Merge branch 'master' into JDK-8266670 - ... and 22 more: https://git.openjdk.java.net/jdk/compare/60434fb7...05cf2d8b - Changes: - all: https://git.openjdk.java.net/jdk/pull/7445/files - new: https://git.openjdk.java.net/jdk/pull/7445/files/ead5911f..05cf2d8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7445=19 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7445=18-19 Stats: 255671 lines in 3128 files changed: 171304 ins; 68829 del; 15538 mod Patch: https://git.openjdk.java.net/jdk/pull/7445.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7445/head:pull/7445 PR: https://git.openjdk.java.net/jdk/pull/7445
