Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:25:54 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line
>> 1371:
>>
>>> 1369: }
>>> 1370:
>>> 1371: default CodeBuilder tableswitch(Label defaultTarget,
>>> List cases) {
>>
>> `switch` seems the one instruction for which an high-level variant (like
>> `trying`) could be useful, as generating code for that can be quite painful.
>
> Nice RFE, thanks.
collected and tracked in the mailing list
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 13:50:07 GMT, Adam Sotona wrote: >> Actually, we also have a ClassfileVersion class, so that could be a better >> place for version numbers? > > There were several iterations of "where to store numeric constants". > It is hard to find them when spread across many classes and duplicities > appear instantly. > Dedicated "Constants" would be another bikeshed with conflicting name. > Co-location in Classfile was the latest decission as it is not just a central > bikeshed, but it also reflect connection with classfile specification. > Please raise that discussion at classfile-api-dev at openjdk.org if necessary. At least for versions its easier to have a singular location because every six months the code needs to be updated. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Thu, 9 Feb 2023 12:45:27 GMT, Maurizio Cimadamore wrote: >> During the fix I found the definition that `ThrowableSig` (as a super-set of >> `ClassTypeSig` and `TypeVarSig`) IS `Signature` - that fact is critical for >> processing. >> For example when `MethodSignature` is serialized >> `ThrowableSig::signatureString` is critical. >> Or `ClassRemapper` depends on the inheritence: >> >> signature.throwableSignatures().stream().map(this::mapSignature).toList() >> >> however `mapSignature` could not be applied on `ThrowableSig` when it does >> not inherit from `Signature`. > > I think I also see where the current hierarchy comes from - e.g. while > `ThrowableSig` is a part of a method signature, it is indeed used by the > production to specify a set of signatures that belong to that set. Thanks for > the patience. No problem, I had to remind myself all the reasons why does it look like Rubik's cube :) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:13:36 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/BufWriter.java line 40: >> >>> 38: * to the end of the buffer, as well as to create constant pool entries. >>> 39: */ >>> 40: public sealed interface BufWriter >> >> What is the relationship between this and ClassReader? Is one the dual of >> the other - e.g. is the intended use for BufWriter to write custom >> attributes, whereas ClassReader reads custom attributes? > > Yes, it is an exposure of low-level API to support custom attributes. I see this topic has moved to classfile-api-dev mailing list. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 11:48:52 GMT, Maurizio Cimadamore wrote: >> Yes, performance is the main reason. >> I'll note to do a fresh differential performance benchmarks with a HashMap. > > thanks I tried HashMap with String keys and it cause performance regressions, however with Utf8Entry initialized with raw Attribute name as the HashMap key - there are no performance regressions. I'm going to push a patch removing all the manually computed HASH_ values and fragile inlined HashMap implementation. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 13:44:13 GMT, Adam Sotona wrote: >> OK, I see your point now, I'll fix it. >> Thanks > > During the fix I found the definition that `ThrowableSig` (as a super-set of > `ClassTypeSig` and `TypeVarSig`) IS `Signature` - that fact is critical for > processing. > For example when `MethodSignature` is serialized > `ThrowableSig::signatureString` is critical. > Or `ClassRemapper` depends on the inheritence: > > signature.throwableSignatures().stream().map(this::mapSignature).toList() > > however `mapSignature` could not be applied on `ThrowableSig` when it does > not inherit from `Signature`. I think I also see where the current hierarchy comes from - e.g. while `ThrowableSig` is a part of a method signature, it is indeed used by the production to specify a set of signatures that belong to that set. Thanks for the patience. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 12:19:35 GMT, Adam Sotona wrote: >>> `Signature.ThrowableSig` is a `Signature` and it is a common super of >>> `ClassTypeSig` and `TypeVarSig`. >> >> I really don't follow here. ThrowableSig is a piece of a method signature, >> which starts with "^" and is followed by either a class or a typevar >> signature. You can never encounter it from the toplevel JavaTypeSignature >> production. It's ok (as I have said) to have a ThrowableSig element in the >> API to model the production, but that element should not be a subtype of >> Signature (at least not if Signature, as you claimed is meant to model the >> JavaTypeSignature production). That is, there's no "is a" relationship >> between JavaTypeSignature and ThrowableSig (at least not that I can see when >> looking at the productions). > > OK, I see your point now, I'll fix it. > Thanks During the fix I found the definition that `ThrowableSig` (as a super-set of `ClassTypeSig` and `TypeVarSig`) IS `Signature` - that fact is critical for processing. For example when `MethodSignature` is serialized `ThrowableSig::signatureString` is critical. Or `ClassRemapper` depends on the inheritence: signature.throwableSignatures().stream().map(this::mapSignature).toList() however `mapSignature` could not be applied on `ThrowableSig` when it does not inherit from `Signature`. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 11:07:08 GMT, Maurizio Cimadamore
wrote:
>> `TypeParam` is not a signature, because it simply is not a signature.
>> Per spec:
>>
>> TypeParameter:
>> Identifier ClassBound {InterfaceBound}
>
>> `Signature.ThrowableSig` is a `Signature` and it is a common super of
>> `ClassTypeSig` and `TypeVarSig`.
>
> I really don't follow here. ThrowableSig is a piece of a method signature,
> which starts with "^" and is followed by either a class or a typevar
> signature. You can never encounter it from the toplevel JavaTypeSignature
> production. It's ok (as I have said) to have a ThrowableSig element in the
> API to model the production, but that element should not be a subtype of
> Signature (at least not if Signature, as you claimed is meant to model the
> JavaTypeSignature production). That is, there's no "is a" relationship
> between JavaTypeSignature and ThrowableSig (at least not that I can see when
> looking at the productions).
OK, I see your point now, I'll fix it.
Thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 07:24:05 GMT, Adam Sotona wrote:
>> **Specification:**
>>
>> MethodSignature:
>> [TypeParameters] ( {JavaTypeSignature} ) Result {ThrowsSignature}
>>
>> Result:
>> JavaTypeSignature
>> VoidDescriptor
>>
>> ThrowsSignature:
>> ^ ClassTypeSignature
>> ^ TypeVariableSignature
>>
>>
>>
>> **Reflect in API mapping:**
>>
>> public sealed interface ClassTypeSig
>> extends RefTypeSig, ThrowableSig
>>
>> and
>>
>> public sealed interface TypeVarSig
>> extends RefTypeSig, ThrowableSig
>>
>> and
>>
>> /**
>> * @return method signature
>> * @param typeParameters signatures for the type parameters
>> * @param exceptions sigantures for the exceptions
>> * @param result signature for the return type
>> * @param arguments signatures for the method arguments
>> */
>> public static MethodSignature of(List
>> typeParameters,
>> List exceptions,
>> Signature result,
>> Signature... arguments) {
>>
>>
>> `Signature.ThrowableSig` is a `Signature` and it is a common super of
>> `ClassTypeSig` and `TypeVarSig`.
>
> `TypeParam` is not a signature, because it simply is not a signature.
> Per spec:
>
> TypeParameter:
> Identifier ClassBound {InterfaceBound}
> `Signature.ThrowableSig` is a `Signature` and it is a common super of
> `ClassTypeSig` and `TypeVarSig`.
I really don't follow here. ThrowableSig is a piece of a method signature,
which starts with "^" and is followed by either a class or a typevar signature.
You can never encounter it from the toplevel JavaTypeSignature production. It's
ok (as I have said) to have a ThrowableSig element in the API to model the
production, but that element should not be a subtype of Signature (at least not
if Signature, as you claimed is meant to model the JavaTypeSignature
production). That is, there's no "is a" relationship between JavaTypeSignature
and ThrowableSig (at least not that I can see when looking at the productions).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:33:46 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/FieldTransform.java line
> 92:
>
>> 90: * @return the field transform
>> 91: */
>> 92: static FieldTransform dropping(Predicate filter) {
>
> Could this be a more general static generic method on ClassfileTransform?
> (but I see CodeTransform doesn't have it - not sure if that's deliberate).
It is deliberate as we didn't found meaningful use case for transformation just
dropping individual code elements.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 11:53:46 GMT, Maurizio Cimadamore
wrote:
>> `BootstrapMethodEntry` is not a constant pool entry, but
>> `BootstrapMethodsAttribute` entry.
>> It might be rather moved under `attribute` package and renamed to
>> `BootstrapMethodInfo` to follow the same pattern as other attributes/infos.
>
> I know it's part of an attribute - but reading the javadoc:
>
> /**
> * Models an entry in the bootstrap method table. The bootstrap method table
> * is stored in the {@code BootstrapMethods} attribute, but is modeled by
> * the {@link ConstantPool}, since the bootstrap method table is logically
> * part of the constant pool.
> */
> ```
>
> And also, seeing the method:
>
> ```
> /**
> * {@return the constant pool associated with this entry}
> */
> ConstantPool constantPool();
>
>
> It seems like the API is doing the (justifiable) trick of making BSMs look
> like if they are first-class CP entries (even if not specified that way in
> the JVMLS). I think that's a fine move, but if we go down that path we should
> be honest, and put this class in constantpool. At least this is my 0.02$.
I see your point. The duality of `BootstrapMethodEntry` and potential confusion
when moved under `attribute` or under `constantpool` package is the reason why
it probably should stay where it is.
However feel free to discuss it at the mailing list.
Thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 07:21:07 GMT, Adam Sotona wrote:
>>> Class `Signature` (aka `JavaTypeSignature`), all subclasses,
>>> `MethodSignature` and `ClassSignature` are designed according to [JVMS
>>> 4.7.9.1
>>> Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1)
>>
>> The production is the same as the one I quoted, but thanks for pointing me
>> at the correct one. So:
>>
>>
>> JavaTypeSignature:
>>ReferenceTypeSignature
>>BaseType
>>
>>
>> and
>>
>>
>> ReferenceTypeSignature:
>>ClassTypeSignature
>>TypeVariableSignature
>>ArrayTypeSignature
>>
>>
>> So, while I can expect that `ArrayTypeSignature` *is a* `Signature` (or
>> `JavaTypeSignature`), I cannot explain why `ThrowsSignature` extends
>> `Signature`. That doesn't seem to follow from the production. That is, if a
>> client obtains a `Signature` and wanted to pattern match, what are the cases
>> it should worry about? I believe the cases are the ones listed above.
>>
>> One thing I missed is that e.g. `TypeParam` is *not* a signature (which is
>> the only case among the nested classes in `Signature`). But
>> `ThrowsSignature`, `TypeArg` and `TypeParam` are signatures even though that
>> doesn't seem to be the case when looking at the production in the JVMS. If
>> we want to keep these fine, but I don't think they should extend
>> `Signature`, either directly or indirectly. That is, `Signature` should be a
>> sealed type with 4 leaves (base-type/array/type var/class-type).
>
> **Specification:**
>
> MethodSignature:
> [TypeParameters] ( {JavaTypeSignature} ) Result {ThrowsSignature}
>
> Result:
> JavaTypeSignature
> VoidDescriptor
>
> ThrowsSignature:
> ^ ClassTypeSignature
> ^ TypeVariableSignature
>
>
>
> **Reflect in API mapping:**
>
> public sealed interface ClassTypeSig
> extends RefTypeSig, ThrowableSig
>
> and
>
> public sealed interface TypeVarSig
> extends RefTypeSig, ThrowableSig
>
> and
>
> /**
> * @return method signature
> * @param typeParameters signatures for the type parameters
> * @param exceptions sigantures for the exceptions
> * @param result signature for the return type
> * @param arguments signatures for the method arguments
> */
> public static MethodSignature of(List typeParameters,
> List exceptions,
> Signature result,
> Signature... arguments) {
>
>
> `Signature.ThrowableSig` is a `Signature` and it is a common super of
> `ClassTypeSig` and `TypeVarSig`.
`TypeParam` is not a signature, because it simply is not a signature.
Per spec:
TypeParameter:
Identifier ClassBound {InterfaceBound}
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 07:21:07 GMT, Adam Sotona wrote:
>>> Class `Signature` (aka `JavaTypeSignature`), all subclasses,
>>> `MethodSignature` and `ClassSignature` are designed according to [JVMS
>>> 4.7.9.1
>>> Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1)
>>
>> The production is the same as the one I quoted, but thanks for pointing me
>> at the correct one. So:
>>
>>
>> JavaTypeSignature:
>>ReferenceTypeSignature
>>BaseType
>>
>>
>> and
>>
>>
>> ReferenceTypeSignature:
>>ClassTypeSignature
>>TypeVariableSignature
>>ArrayTypeSignature
>>
>>
>> So, while I can expect that `ArrayTypeSignature` *is a* `Signature` (or
>> `JavaTypeSignature`), I cannot explain why `ThrowsSignature` extends
>> `Signature`. That doesn't seem to follow from the production. That is, if a
>> client obtains a `Signature` and wanted to pattern match, what are the cases
>> it should worry about? I believe the cases are the ones listed above.
>>
>> One thing I missed is that e.g. `TypeParam` is *not* a signature (which is
>> the only case among the nested classes in `Signature`). But
>> `ThrowsSignature`, `TypeArg` and `TypeParam` are signatures even though that
>> doesn't seem to be the case when looking at the production in the JVMS. If
>> we want to keep these fine, but I don't think they should extend
>> `Signature`, either directly or indirectly. That is, `Signature` should be a
>> sealed type with 4 leaves (base-type/array/type var/class-type).
>
> **Specification:**
>
> MethodSignature:
> [TypeParameters] ( {JavaTypeSignature} ) Result {ThrowsSignature}
>
> Result:
> JavaTypeSignature
> VoidDescriptor
>
> ThrowsSignature:
> ^ ClassTypeSignature
> ^ TypeVariableSignature
>
>
>
> **Reflect in API mapping:**
>
> public sealed interface ClassTypeSig
> extends RefTypeSig, ThrowableSig
>
> and
>
> public sealed interface TypeVarSig
> extends RefTypeSig, ThrowableSig
>
> and
>
> /**
> * @return method signature
> * @param typeParameters signatures for the type parameters
> * @param exceptions sigantures for the exceptions
> * @param result signature for the return type
> * @param arguments signatures for the method arguments
> */
> public static MethodSignature of(List typeParameters,
> List exceptions,
> Signature result,
> Signature... arguments) {
>
>
> `Signature.ThrowableSig` is a `Signature` and it is a common super of
> `ClassTypeSig` and `ClassTypeSig`.
`TypeParam` is not a signature, because it simply is not a signature.
Per spec:
TypeParameter:
Identifier ClassBound {InterfaceBound}
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 15:47:25 GMT, Maurizio Cimadamore
wrote:
>> Class `Signature` (aka `JavaTypeSignature`), all subclasses,
>> `MethodSignature` and `ClassSignature` are designed according to [JVMS
>> 4.7.9.1
>> Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1)
>
>> Class `Signature` (aka `JavaTypeSignature`), all subclasses,
>> `MethodSignature` and `ClassSignature` are designed according to [JVMS
>> 4.7.9.1
>> Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1)
>
> The production is the same as the one I quoted, but thanks for pointing me at
> the correct one. So:
>
>
> JavaTypeSignature:
>ReferenceTypeSignature
>BaseType
>
>
> and
>
>
> ReferenceTypeSignature:
>ClassTypeSignature
>TypeVariableSignature
>ArrayTypeSignature
>
>
> So, while I can expect that `ArrayTypeSignature` *is a* `Signature` (or
> `JavaTypeSignature`), I cannot explain why `ThrowsSignature` extends
> `Signature`. That doesn't seem to follow from the production. That is, if a
> client obtains a `Signature` and wanted to pattern match, what are the cases
> it should worry about? I believe the cases are the ones listed above.
>
> One thing I missed is that e.g. `TypeParam` is *not* a signature (which is
> the only case among the nested classes in `Signature`). But
> `ThrowsSignature`, `TypeArg` and `TypeParam` are signatures even though that
> doesn't seem to be the case when looking at the production in the JVMS. If we
> want to keep these fine, but I don't think they should extend `Signature`,
> either directly or indirectly. That is, `Signature` should be a sealed type
> with 4 leaves (base-type/array/type var/class-type).
**Specification:**
MethodSignature:
[TypeParameters] ( {JavaTypeSignature} ) Result {ThrowsSignature}
Result:
JavaTypeSignature
VoidDescriptor
ThrowsSignature:
^ ClassTypeSignature
^ TypeVariableSignature
**Reflect in API mapping:**
public sealed interface ClassTypeSig
extends RefTypeSig, ThrowableSig
and
public sealed interface TypeVarSig
extends RefTypeSig, ThrowableSig
and
/**
* @return method signature
* @param typeParameters signatures for the type parameters
* @param exceptions sigantures for the exceptions
* @param result signature for the return type
* @param arguments signatures for the method arguments
*/
public static MethodSignature of(List typeParameters,
List exceptions,
Signature result,
Signature... arguments) {
`Signature.ThrowableSig` is a `Signature` and it is a common super of
`ClassTypeSig` and `ClassTypeSig`.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 14:51:06 GMT, Adam Sotona wrote: > Class `Signature` (aka `JavaTypeSignature`), all subclasses, > `MethodSignature` and `ClassSignature` are designed according to [JVMS > 4.7.9.1 > Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1) The production is the same as the one I quoted, but thanks for pointing me at the correct one. So: JavaTypeSignature: ReferenceTypeSignature BaseType and ReferenceTypeSignature: ClassTypeSignature TypeVariableSignature ArrayTypeSignature So, while I can expect that `ArrayTypeSignature` *is a* `Signature` (or `JavaTypeSignature`), I cannot explain why `ThrowsSignature` extends `Signature`. That doesn't seem to follow from the production. That is, if a client obtains a `Signature` and wanted to pattern match, what are the cases it should worry about? I believe the cases are the ones listed above. One thing I missed is that e.g. `TypeParam` is *not* a signature (which is the only case among the nested classes in `Signature`). But `ThrowsSignature`, `TypeArg` and `TypeParam` are signatures even though that doesn't seem to be the case when looking at the production in the JVMS. If we want to keep these fine, but I don't think they should extend `Signature`, either directly or indirectly. That is, `Signature` should be a sealed type with 4 leaves (base-type/array/type var/class-type). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:10:43 GMT, Maurizio Cimadamore wrote: >> The confusion come from simplified name. Signature probably should be called >> JavaTypeSignature according to the spec. ClassSignature and MethodSignature >> could not extend it, as it would not respect the reality. Each of them are >> completely different species. Signature (or JavaTypeSignature) on the other >> side has many sub-types. > > I think you mean `TypeSignature` from JLS 4.3.4 ? If so, I get it. To me it > looks as if "Signature" really means "TypeSignature" - then it would make > sense as to why `MethodSignature::arguments` returns a `List` > (e.g. because it should be `List`, as in the spec). > > Also, from a modelling perspective, I see other problems too - in the JVMS, > type signatures are defined as follows: > > > TypeSignature: > FieldTypeSignature > BaseType > ``` > And: > > > FieldTypeSignature: > ClassTypeSignature > ArrayTypeSignature > TypeVariableSignature > > > So I'd roughly expect a type with 4 subclasses (type, then class <: type, > array <: type, tvar <: type). > > But the Signature class currently has other subclasses too - such as > `ThrowableSig` - which is really only an (optional) part of method > signatures. And, similarly, it also has `TypeParam`, which is a part of > method/class signature - but is _not_ a type signature per se. > > Summing up, it seems to me that the naming issue (which is not that > important) hides a bigger modelling issue. Class `Signature` (aka `JavaTypeSignature`), all subclasses, `MethodSignature` and `ClassSignature` are designed according to [JVMS 4.7.9.1 Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 14:14:50 GMT, Maurizio Cimadamore wrote: >> The relation is that each `Attribute` is applicable in N >> `AttributedElements` and not vice versa. >> For example `ClassModel::attributedElementKind` returns `CLASS` and for >> example >> `Attributes.RUNTIME_INVISIBLE_TYPE_ANNOTATIONS::applicableAttributeKinds` >> returns `Set.of(CLASS, METHOD, FIELD, CODE_ATTRIBUTE, RECORD_COMPONENT)`, so >> we know it is applicable. >> >> However I'll try to re-visit this part of API if needed at all. > > I understand what the API, in its current form, wants to do. IMHO, the same > functionality would be better expressed if the API had a way to say: > > * each attribute has a _target_, where target can be a set of CLASS, METHOD, > ... > * an attribute mapper supports a set of attribute targets > * an attributed element also supports a set of attribute targets > > E.g. the target of the attribute is the concept to model - then > AttributeMapper and AttributedElement just happen to expose a set of > supported targets, which clients can query if needed. I've prepared pull request to jdk-sandbox with complete removal of the AttributedElement.Kind and sent following note to classfile-api-dev at openjdk.org : I would like to propose removal of AttributedElement.Kind across all Classfile API. The AttributedElement.Kind models Attributes “where applicable” and it is a duplication of each Attribute extending ClassElement, MethodElement, CodeElement, etc… Classfile API is not actively using the AttributedElement.Kind except for parsing, where inappropriate AttributedElement.Kind is resolved as UnknownAttribute. Following proposal removes all usages of AttributedElement.Kind from Classfile API: https://github.com/openjdk/jdk-sandbox/pull/48/files Please let me know is there are any objections. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:59:32 GMT, Adam Sotona wrote: >> Still, there seems to be a modelling issue here. The property of "where >> could this attribute go" is a property of the attribute. Of course, for >> usability reason, an AttributedElement might expose a predicate saying "I >> only accept attributes of these kinds". But it seems to me as if the API has >> this relationship backwards, due to _where_ the Kind interface is being >> defined. I think if `Kind` was defined in `AttributeMapper` it would be a >> tad easier to understand. And, perhaps, the `attributedElementKind` name >> should be changed to something like `applicableAttributeKinds` or something >> like that. > > The relation is that each `Attribute` is applicable in N `AttributedElements` > and not vice versa. > For example `ClassModel::attributedElementKind` returns `CLASS` and for > example > `Attributes.RUNTIME_INVISIBLE_TYPE_ANNOTATIONS::applicableAttributeKinds` > returns `Set.of(CLASS, METHOD, FIELD, CODE_ATTRIBUTE, RECORD_COMPONENT)`, so > we know it is applicable. > > However I'll try to re-visit this part of API if needed at all. I understand what the API, in its current form, wants to do. IMHO, the same functionality would be better expressed if the API had a way to say: * each attribute has a _target_, where target can be a set of CLASS, METHOD, ... * an attribute mapper supports a set of attribute targets * an attributed element also supports a set of attribute targets E.g. the target of the attribute is the concept to model - then AttributeMapper and AttributedElement just happen to expose a set of supported targets, which clients can query if needed. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:34:50 GMT, Maurizio Cimadamore wrote: >> `AttributedElement::attributedElementKind` identifies the one kind of the >> attributes holder. >> The "places where an attribute can appear" is available through >> `AttributeMapper::whereApplicable` and matched against >> `AttributedElement::attributedElementKind`. >> We may consider to hide or remove this auxiliary method, as >> `AttributedElement::attributedElementKind` might be computed from the >> ClassfileElement instance type. > > Still, there seems to be a modelling issue here. The property of "where could > this attribute go" is a property of the attribute. Of course, for usability > reason, an AttributedElement might expose a predicate saying "I only accept > attributes of these kinds". But it seems to me as if the API has this > relationship backwards, due to _where_ the Kind interface is being defined. I > think if `Kind` was defined in `AttributeMapper` it would be a tad easier to > understand. And, perhaps, the `attributedElementKind` name should be changed > to something like `applicableAttributeKinds` or something like that. The relation is that each `Attribute` is applicable in N `AttributedElements` and not vice versa. For example `ClassModel::attributedElementKind` returns `CLASS` and for example `Attributes.RUNTIME_INVISIBLE_TYPE_ANNOTATIONS::applicableAttributeKinds` returns `Set.of(CLASS, METHOD, FIELD, CODE_ATTRIBUTE, RECORD_COMPONENT)`, so we know it is applicable. However I'll try to re-visit this part of API if needed at all. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:14:40 GMT, Maurizio Cimadamore wrote: >> On the contrary, it has been deduplicated. Opcode is referencing numeric >> constants stored in Classfile. > > sure, but my question is - once you have a nice enum that is 1-1 with the > opcodes - why would a client want to use the low-level opcode bytes? > Shouldn't the API only deal with Opcodes? (and, in the rare occurrence where > a client wants to really know the int value of an opcode, they can do e.g. > `PUTSTATIC.bytecode()`) It is the question of co-location of all numeric constants in one place in Classfile (or not). I would agree with your proposal if the architectural decision is to decentralise the numeric constants and reduce Classfile footprint. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:23:05 GMT, Adam Sotona wrote: >> Uhm - I can't see these usages... something seems to be off with my IDE >> configuration. I did a grep and I now saw the uses. That said, having the >> Kind/Location inside AttributedElement still looks weird to me. The "places >> where an attribute can appear" is a property of an `Attribute`, not of an >> attributed element (which is used to model elements which can have >> attributes). > > `AttributedElement::attributedElementKind` identifies the one kind of the > attributes holder. > The "places where an attribute can appear" is available through > `AttributeMapper::whereApplicable` and matched against > `AttributedElement::attributedElementKind`. > We may consider to hide or remove this auxiliary method, as > `AttributedElement::attributedElementKind` might be computed from the > ClassfileElement instance type. Still, there seems to be a modelling issue here. The property of "where could this attribute go" is a property of the attribute. Of course, for usability reason, an AttributedElement might expose a predicate saying "I only accept attributes of these kinds". But it seems to me as if the API has this relationship backwards, due to _where_ the Kind interface is being defined. I think if `Kind` was defined in `AttributeMapper` it would be a tad easier to understand. And, perhaps, the `attributedElementKind` name should be changed to something like `applicableAttributeKinds` or something like that. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 11:48:28 GMT, Maurizio Cimadamore wrote: >> There are at least 72 usages of AttributedElement.Kind across the Classfile >> API. >> Do you suggest to rename it to something else (for example Location)? > > Uhm - I can't see these usages... something seems to be off with my IDE > configuration. I did a grep and I now saw the uses. That said, having the > Kind/Location inside AttributedElement still looks weird to me. The "places > where an attribute can appear" is a property of an `Attribute`, not of an > attributed element (which is used to model elements which can have > attributes). `AttributedElement::attributedElementKind` identifies the one kind of the attributes holder. The "places where an attribute can appear" is available through `AttributeMapper::whereApplicable` and matched against `AttributedElement::attributedElementKind`. We may consider to hide or remove this auxiliary method, as `AttributedElement::attributedElementKind` might be computed from the ClassfileElement instance type. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:35:43 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/Opcode.java line 39:
>>
>>> 37: */
>>> 38: public enum Opcode {
>>> 39: NOP(Classfile.NOP, 1, Kind.NOP),
>>
>> This also duplicates the constants in classfile...
>
> On the contrary, it has been deduplicated. Opcode is referencing numeric
> constants stored in Classfile.
sure, but my question is - once you have a nice enum that is 1-1 with the
opcodes - why would a client want to use the low-level opcode bytes? Shouldn't
the API only deal with Opcodes? (and, in the rare occurrence where a client
wants to really know the int value of an opcode, they can do e.g.
`PUTSTATIC.bytecode()`)
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:32:12 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/ClassSignature.java line >> 34: >> >>> 32: * Models the generic signature of a class file, as defined by JVMS >>> 4.7.9. >>> 33: */ >>> 34: public sealed interface ClassSignature >> >> It is a bit odd to have Signature and XYZSignature in the API with the >> latter not extending the former. I think I get what the API is aiming for >> though - e.g. Signature is for modelling low-level "portions" of the >> signature attributes, whereas ClassSignature, MethodSignature and friends >> are there to model the signature attribute attached to a class, method, etc. > > The confusion come from simplified name. Signature probably should be called > JavaTypeSignature according to the spec. ClassSignature and MethodSignature > could not extend it, as it would not respect the reality. Each of them are > completely different species. Signature (or JavaTypeSignature) on the other > side has many sub-types. I think you mean `TypeSignature` from JLS 4.3.4 ? If so, I get it. To me it looks as if "Signature" really means "TypeSignature" - then it would make sense as to why `MethodSignature::arguments` returns a `List` (e.g. because it should be `List`, as in the spec). Also, from a modelling perspective, I see other problems too - in the JVMS, type signatures are defined as follows: TypeSignature: FieldTypeSignature BaseType ``` And: FieldTypeSignature: ClassTypeSignature ArrayTypeSignature TypeVariableSignature So I'd roughly expect a type with 4 subclasses (type, then class <: type, array <: type, tvar <: type). But the Signature class currently has other subclasses too - such as `ThrowableSig` - which is really only an (optional) part of method signatures. And, similarly, it also has `TypeParam`, which is a part of method/class signature - but is _not_ a type signature per se. Summing up, it seems to me that the naming issue (which is not that important) hides a bigger modelling issue. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:09:08 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/BootstrapMethodEntry.java
>> line 41:
>>
>>> 39: * part of the constant pool.
>>> 40: */
>>> 41: public sealed interface BootstrapMethodEntry
>>
>> Usages of this seem all to fall into the `constantpool` package - does this
>> interface belong there?
>
> `BootstrapMethodEntry` is not a constant pool entry, but
> `BootstrapMethodsAttribute` entry.
> It might be rather moved under `attribute` package and renamed to
> `BootstrapMethodInfo` to follow the same pattern as other attributes/infos.
I know it's part of an attribute - but reading the javadoc:
/**
* Models an entry in the bootstrap method table. The bootstrap method table
* is stored in the {@code BootstrapMethods} attribute, but is modeled by
* the {@link ConstantPool}, since the bootstrap method table is logically
* part of the constant pool.
*/
```
And also, seeing the method:
```
/**
* {@return the constant pool associated with this entry}
*/
ConstantPool constantPool();
It seems like the API is doing the (justifiable) trick of making BSMs look like
if they are first-class CP entries (even if not specified that way in the
JVMLS). I think that's a fine move, but if we go down that path we should be
honest, and put this class in constantpool. At least this is my 0.02$.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 13:55:59 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/AttributedElement.java
>> line 94:
>>
>>> 92: * are permitted.
>>> 93: */
>>> 94: enum Kind {
>>
>> Not sure how to interpret this. This seems to refer to an attribute - e.g.
>> where is an attribute allowed - rather than to the element (e.g. the
>> declaration to which the attribute is attached). All the constants are
>> unused, so hard to make sense of how this is used.
>
> There are at least 72 usages of AttributedElement.Kind across the Classfile
> API.
> Do you suggest to rename it to something else (for example Location)?
Uhm - I can't see these usages... something seems to be off with my IDE
configuration. I did a grep and I now saw the uses. That said, having the
Kind/Location inside AttributedElement still looks weird to me. The "places
where an attribute can appear" is a property of an `Attribute`, not of an
attributed element (which is used to model elements which can have attributes).
>> src/java.base/share/classes/jdk/internal/classfile/Attributes.java line 774:
>>
>>> 772: */
>>> 773: public static AttributeMapper standardAttribute(Utf8Entry name)
>>> {
>>> 774: int hash = name.hashCode();
>>
>> If we have a map, why do we need this "inlined" map here? Performance
>> reasons?
>
> Yes, performance is the main reason.
> I'll note to do a fresh differential performance benchmarks with a HashMap.
thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 12:41:44 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
>> 75:
>>
>>> 73: * The kind of target on which the annotation appears.
>>> 74: */
>>> 75: public enum TargetType {
>>
>> My IDE says this enum is not being used. I tend to believe it, since the
>> TargetInfo sealed interface also seems to model the same thing?
>
> There is only one TargetInfo for all TargetTypes, so instead of 22
> sub-interfaces of TargetInfo, the instance of TargetType enum is hold in
> TargetInfo.
Ok, I see that now - for some reason the IDE could not find the usage... thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:25:17 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 333:
>
>> 331: * @see #catchingAll
>> 332: */
>> 333: CatchBuilder catching(ClassDesc exceptionType,
>> Consumer catchHandler);
>
> I imagine there are name clashes with Java keyword, hence the `ing` in the
> names. That said, this should probably revisited in a later bikeshedding
> round - as in the current form, the code builder API has most method names
> that are nouns (the thing being built) but it also has some verbs in there
> (trying, catching) which seem odd.
Please raise the naming convention discussion at classfile-api-dev at
openjdk.org
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/Opcode.java line 39:
>
>> 37: */
>> 38: public enum Opcode {
>> 39: NOP(Classfile.NOP, 1, Kind.NOP),
>
> This also duplicates the constants in classfile...
On the contrary, it has been deduplicated. Opcode is referencing numeric
constants stored in Classfile.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:37:43 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/ClassSignature.java line > 34: > >> 32: * Models the generic signature of a class file, as defined by JVMS >> 4.7.9. >> 33: */ >> 34: public sealed interface ClassSignature > > It is a bit odd to have Signature and XYZSignature in the API with the latter > not extending the former. I think I get what the API is aiming for though - > e.g. Signature is for modelling low-level "portions" of the signature > attributes, whereas ClassSignature, MethodSignature and friends are there to > model the signature attribute attached to a class, method, etc. The confusion come from simplified name. Signature probably should be called JavaTypeSignature according to the spec. ClassSignature and MethodSignature could not extend it, as it would not respect the reality. Each of them are completely different species. Signature (or JavaTypeSignature) on the other side has many sub-types. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:11:41 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/ClassModel.java line 104:
>
>> 102: * found
>> 103: */
>> 104: default List verify(Consumer debugOutput) {
>
> not super sure whether `verify` belongs here - could be another component in
> the `components` package?
Classfile API by default does not verify produced or transformed bytecode, so
this is more a question if we want to have verification an integral part of the
API or a standalone tool.
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 613:
>
>> 611: }
>> 612:
>> 613: default CodeBuilder labelBinding(Label label) {
>
> Maybe just `bind` or `bindLabel` ?
It has been discussed (and changed) many times. Please raise this discussion at
classfile-api-dev at openjdk.org
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 1371:
>
>> 1369: }
>> 1370:
>> 1371: default CodeBuilder tableswitch(Label defaultTarget,
>> List cases) {
>
> `switch` seems the one instruction for which an high-level variant (like
> `trying`) could be useful, as generating code for that can be quite painful.
Nice RFE, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:59:53 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/BufWriter.java line 40: > >> 38: * to the end of the buffer, as well as to create constant pool entries. >> 39: */ >> 40: public sealed interface BufWriter > > What is the relationship between this and ClassReader? Is one the dual of the > other - e.g. is the intended use for BufWriter to write custom attributes, > whereas ClassReader reads custom attributes? Yes, it is an exposure of low-level API to support custom attributes. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:58:04 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/BootstrapMethodEntry.java > line 41: > >> 39: * part of the constant pool. >> 40: */ >> 41: public sealed interface BootstrapMethodEntry > > Usages of this seem all to fall into the `constantpool` package - does this > interface belong there? `BootstrapMethodEntry` is not a constant pool entry, but `BootstrapMethodsAttribute` entry. It might be rather moved under `attribute` package and renamed to `BootstrapMethodInfo` to follow the same pattern as other attributes/infos. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:56:45 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/Attributes.java line 774:
>
>> 772: */
>> 773: public static AttributeMapper standardAttribute(Utf8Entry name) {
>> 774: int hash = name.hashCode();
>
> If we have a map, why do we need this "inlined" map here? Performance reasons?
Yes, performance is the main reason.
I'll note to do a fresh differential performance benchmarks with a HashMap.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:52:49 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/AttributedElement.java
> line 94:
>
>> 92: * are permitted.
>> 93: */
>> 94: enum Kind {
>
> Not sure how to interpret this. This seems to refer to an attribute - e.g.
> where is an attribute allowed - rather than to the element (e.g. the
> declaration to which the attribute is attached). All the constants are
> unused, so hard to make sense of how this is used.
There are at least 72 usages of AttributedElement.Kind across the Classfile API.
Do you suggest to rename it to something else (for example Location)?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:07:27 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 346: >> >>> 344: public static final int MAGIC_NUMBER = 0xCAFEBABE; >>> 345: >>> 346: public static final int NOP = 0; >> >> Not sure how I feel about the constants being here. It seems to me that they >> can be moved to more appropriate places - e.g. Instructor, TypeAnnotation >> (for the TAT ones), ConstantPool (for the TAG ones). >> >> The classfile versions, OTOH, do seem to belong here. > > Actually, we also have a ClassfileVersion class, so that could be a better > place for version numbers? There were several iterations of "where to store numeric constants". It is hard to find them when spread across many classes and duplicities appear instantly. Dedicated "Constants" would be another bikeshed with conflicting name. Co-location in Classfile was the latest decission as it is not just a central bikeshed, but it also reflect connection with classfile specification. Please raise that discussion at classfile-api-dev at openjdk.org if necessary. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:43:22 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 58:
>
>> 56: * Main entry points for parsing, transforming, and generating
>> classfiles.
>> 57: */
>> 58: public class Classfile {
>
> class or interface? (bikeshed, I realize)
yes, a bikeshed
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 197:
>
>> 195: * @return the classfile bytes
>> 196: */
>> 197: public static byte[] build(ClassDesc thisClass,
>
> The "build" methods here seem regular - e.g. build { class, module } x { byte
> array, path }. As a future improvement, perhaps capturing the "sink" of a
> build process might be helpful - so that you can avoid overloads between
> array and path variants. (e.g. `build( toByteArray(arr))`, or
> `build(...).toByteArray(...)`.
Classfile::build always return byte array and there is no "sink" model inside.
Classfile::buildTo is frequently used extension; a wrapper calling build and
writing the byte array using Files::write.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:37:55 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/package-info.java line 39:
>
>> 37: *
>> 38: * {@snippet lang=java :
>> 39: * ClassModel cm = ClassModel.of(bytes);
>
> There are several references to `ClassModel::of` (here and elsewhere), but
> this seems to have been moved to `ClassFile::parse`
will fix, thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:32:37 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/attribute/SignatureAttribute.java > line 66: > >> 64: >> 65: /** >> 66: * Parse the siganture as a method signature. > > typo here `siganture` - check the entire class will fix, thanks - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:20:19 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line > 617: > >> 615: } >> 616: >> 617: public static final TypePathComponent ARRAY = new >> UnboundAttribute.TypePathComponentImpl(Kind.ARRAY, 0); > > `public static final` is redundant here (it's an interface) - please check > these (I've seen others). E.g. all `public` modifier for types nested in > interfaces are redundant as well. yes, will fix and search for other such cases, thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:23:51 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
> 641:
>
>> 639: int typeArgumentIndex();
>> 640:
>> 641: static TypePathComponent of(int typePathKind, int
>> typeArgumentIndex) {
>
> Should this take a `Kind` instead of an `int`?
Yes, will fix, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:22:32 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
> 75:
>
>> 73: * The kind of target on which the annotation appears.
>> 74: */
>> 75: public enum TargetType {
>
> My IDE says this enum is not being used. I tend to believe it, since the
> TargetInfo sealed interface also seems to model the same thing?
There is only one TargetInfo for all TargetTypes, so instead of 22
sub-interfaces of TargetInfo, the instance of TargetType enum is hold in
TargetInfo.
> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
> 577:
>
>> 575:
>> 576: /**
>> 577: * type_path.path.
>
> The javadoc in this class seems off
will fix, thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:46:32 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 346: > >> 344: public static final int MAGIC_NUMBER = 0xCAFEBABE; >> 345: >> 346: public static final int NOP = 0; > > Not sure how I feel about the constants being here. It seems to me that they > can be moved to more appropriate places - e.g. Instructor, TypeAnnotation > (for the TAT ones), ConstantPool (for the TAG ones). > > The classfile versions, OTOH, do seem to belong here. Actually, we also have a ClassfileVersion class, so that could be a better place for version numbers? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 14:31:24 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Classfile API moved under jdk.internal.classfile package
src/java.base/share/classes/jdk/internal/classfile/AttributedElement.java line
94:
> 92: * are permitted.
> 93: */
> 94: enum Kind {
Not sure how to interpret this. This seems to refer to an attribute - e.g.
where is an attribute allowed - rather than to the element (e.g. the
declaration to which the attribute is attached). All the constants are unused,
so hard to make sense of how this is used.
src/java.base/share/classes/jdk/internal/classfile/Attributes.java line 774:
> 772: */
> 773: public static AttributeMapper standardAttribute(Utf8Entry name) {
> 774: int hash = name.hashCode();
If we have a map, why do we need this "inlined" map here? Performance reasons?
src/java.base/share/classes/jdk/internal/classfile/BootstrapMethodEntry.java
line 41:
> 39: * part of the constant pool.
> 40: */
> 41: public sealed interface BootstrapMethodEntry
Usages of this seem all to fall into the `constantpool` package - does this
interface belong there?
src/java.base/share/classes/jdk/internal/classfile/BufWriter.java line 40:
> 38: * to the end of the buffer, as well as to create constant pool entries.
> 39: */
> 40: public sealed interface BufWriter
What is the relationship between this and ClassReader? Is one the dual of the
other - e.g. is the intended use for BufWriter to write custom attributes,
whereas ClassReader reads custom attributes?
src/java.base/share/classes/jdk/internal/classfile/ClassModel.java line 104:
> 102: * found
> 103: */
> 104: default List verify(Consumer debugOutput) {
not super sure whether `verify` belongs here - could be another component in
the `components` package?
src/java.base/share/classes/jdk/internal/classfile/ClassSignature.java line 34:
> 32: * Models the generic signature of a class file, as defined by JVMS 4.7.9.
> 33: */
> 34: public sealed interface ClassSignature
It is a bit odd to have Signature and XYZSignature in the API with the latter
not extending the former. I think I get what the API is aiming for though -
e.g. Signature is for modelling low-level "portions" of the signature
attributes, whereas ClassSignature, MethodSignature and friends are there to
model the signature attribute attached to a class, method, etc.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 58:
> 56: * Main entry points for parsing, transforming, and generating classfiles.
> 57: */
> 58: public class Classfile {
class or interface? (bikeshed, I realize)
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 197:
> 195: * @return the classfile bytes
> 196: */
> 197: public static byte[] build(ClassDesc thisClass,
The "build" methods here seem regular - e.g. build { class, module } x { byte
array, path }. As a future improvement, perhaps capturing the "sink" of a build
process might be helpful - so that you can avoid overloads between array and
path variants. (e.g. `build( toByteArray(arr))`, or
`build(...).toByteArray(...)`.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 346:
> 344: public static final int MAGIC_NUMBER = 0xCAFEBABE;
> 345:
> 346: public static final int NOP = 0;
Not sure how I feel about the constants being here. It seems to me that they
can be moved to more appropriate places - e.g. Instructor, TypeAnnotation (for
the TAT ones), ConstantPool (for the TAG ones).
The classfile versions, OTOH, do seem to belong here.
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 202:
> 200: default CodeBuilder block(Consumer handler) {
> 201: Label breakLabel = newLabel();
> 202: BlockCodeBuilderImpl child = new BlockCodeBuilderIm
Re: RFR: 8294982: Implementation of Classfile API [v12]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Classfile API moved under jdk.internal.classfile package - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/80213e61..8df1dc21 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=10-11 Stats: 21514 lines in 401 files changed: 10054 ins; 10054 del; 1406 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
