Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 14:24:18 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
>> line 47:
>>
>>> 45: * {@return the start of the instruction range}
>>> 46: */
>>> 47: Label startScope();
>>
>> I noticed that this pattern of start/endScope is here, but also in
>> ExceptionCatch, LocalVariable and LocalVariableType. Consider using a common
>> interface for "instructions that are associated with a range".
>
> That is interesting RFE, thanks.
collected and tracked in the mailing list
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 14:12:21 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java >> line 63: >> >>> 61: * aload_0}). >>> 62: */ >>> 63: sealed interface IntrinsicConstantInstruction extends >>> ConstantInstruction >> >> I'm not super sure of the fine-grained distinction here. The constant pool >> variant is interesting (as you can ask for the associated constant entry) - >> but the distinction between intrinsics vs. argument seems rather weak. > > They significantly differ in instruction formats and instruction format > distinction is critical for some use cases. I think this distinction is appropriate for the level of modeling. CodeBuilder::constantInstruction(ConstantDesc value) is very useful in selecting the most appropriate specific constant instruction. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:07:32 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/MethodHandleEntry.java
> line 40:
>
>> 38:
>> 39: /**
>> 40: * {@return the reference kind of this method handle}
>
> Where are the constants that can be used to decode the MH kind? Perhaps a
> reference from the javadoc could be helpful.
Fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/PoolEntry.java
> line 55:
>
>> 53: * {@return the number of constant pool slots this entry consumes}
>> 54: */
>> 55: int poolEntries();
>
> maybe `width` ?
Yes, it sounds better. Fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 16 Feb 2023 10:48:49 GMT, Adam Sotona wrote: >> The main difference is that if the given entry is not directly applicable >> (when it comes from 3rd constant pool or when the pool is not shared during >> transformation) - a new or matching entry to the target pool is returned. >> This is not a builder pattern, this is projection of an entry to the target >> CP and it also prevents duplicates. > > `maybeClone` is not the main avenue, it is rather a helper method and I'll > look at details if it is necessary too expose it. `ConstantPoolBuilder::maybeClone` and `PoolEntry::clone` have been pulled from API to implementation - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 16 Feb 2023 10:25:04 GMT, Maurizio Cimadamore wrote: >> I'm not quite sure what exactly do you propose. >> `ConstantPool` should not accept anything as it is read-only, so "accept" >> would be confusing. >> `ConstantPoolBuilder::maybeClone` is rather a `Function`, where the name >> might be changed to `ConstantPoolBuilder::apply`. >> However there are so many "accepts" and "applies" across the API, that >> reducing API verbosity to just these functional terms might significantly >> decrease readability. > > As I read the javadoc (I have not looked at impl yet), this method can > effectively be used to add an entry to the constant pool (if the entry does > not yet exist - in which case existing entry is returned). > > After having looked at the implementation a bit, I think my assumption is > correct - e.g. when calling `getfield` on a code builder, the FieldRef passed > to the code builder is added to the constant pool using `maybeClone`. > > So, in my mind at least, this `maybeClone` is the main avenue by which new > constant pool entries are added. In builders we have a `with` method - I > think a `with` method would also be ok here, given what the method does (ok, > there is the wrinkle that, if the entry already exists it is not added, but > that seems more of an optimization to avoid duplicates). The main difference is that if the given entry is not directly applicable (when it comes from 3rd constant pool or when the pool is not shared during transformation) - a new or matching entry to the target pool is returned. This is not a builder pattern, this is projection of an entry to the target CP and it also prevents duplicates. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 16 Feb 2023 10:46:23 GMT, Adam Sotona wrote: >> As I read the javadoc (I have not looked at impl yet), this method can >> effectively be used to add an entry to the constant pool (if the entry does >> not yet exist - in which case existing entry is returned). >> >> After having looked at the implementation a bit, I think my assumption is >> correct - e.g. when calling `getfield` on a code builder, the FieldRef >> passed to the code builder is added to the constant pool using `maybeClone`. >> >> So, in my mind at least, this `maybeClone` is the main avenue by which new >> constant pool entries are added. In builders we have a `with` method - I >> think a `with` method would also be ok here, given what the method does (ok, >> there is the wrinkle that, if the entry already exists it is not added, but >> that seems more of an optimization to avoid duplicates). > > The main difference is that if the given entry is not directly applicable > (when it comes from 3rd constant pool or when the pool is not shared during > transformation) - a new or matching entry to the target pool is returned. > This is not a builder pattern, this is projection of an entry to the target > CP and it also prevents duplicates. `maybeClone` is not the main avenue, it is rather a helper method and I'll look at details if it is necessary too expose it. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 08:20:19 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java >> line 537: >> >>> 535: * @param the type of the entry >>> 536: */ >>> 537: T maybeClone(T entry); >> >> This feels a more primitive operation than the name suggests. Have you >> considered making ConstantPool extend Consumer and call this >> "accept" ? > > I'm not quite sure what exactly do you propose. > `ConstantPool` should not accept anything as it is read-only, so "accept" > would be confusing. > `ConstantPoolBuilder::maybeClone` is rather a `Function`, where the name > might be changed to `ConstantPoolBuilder::apply`. > However there are so many "accepts" and "applies" across the API, that > reducing API verbosity to just these functional terms might significantly > decrease readability. As I read the javadoc (I have not looked at impl yet), this method can effectively be used to add an entry to the constant pool (if the entry does not yet exist - in which case existing entry is returned). After having looked at the implementation a bit, I think my assumption is correct - e.g. when calling `getfield` on a code builder, the FieldRef passed to the code builder is added to the constant pool using `maybeClone`. So, in my mind at least, this `maybeClone` is the main avenue by which new constant pool entries are added. In builders we have a `with` method - I think a `with` method would also be ok here, given what the method does (ok, there is the wrinkle that, if the entry already exists it is not added, but that seems more of an optimization to avoid duplicates). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Fri, 10 Feb 2023 11:16:22 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/constantpool/Utf8Entry.java >> line 47: >> >>> 45: * @param s the string to compare to >>> 46: */ >>> 47: boolean equalsString(String s); >> >> Whatif the provided string contains character that are not representable in >> Utf8? Should we throw (e.g. `IllegalArgumentException`) ? > > I would rather expect `false` value returned, however this case is probably > not covered by tests. You almost got me, I had to think about it for a while :) Any String is always representable in Utf8 (or modified Utf8 used in classfile). It is completely valid to store even 4-byte Unicode characters in Utf8Entry and match them with (Utf16 encoded) String. I'll add such String to the existing test. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:05:10 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
> line 67:
>
>> 65: * {@return whether this member is an interface method}
>> 66: */
>> 67: boolean isInterface();
>
> I'd prefer to see this to `MethodRefEntry`. But again, there's an entry for
> interface methods, so not sure how much this is needed.
The same as above, it is possible to remove it and the two use cases (in
`ConcreteMethodHandleEntry::asSymbol` and `ClassPrinterImpl`) replace with
`instanceof InterfaceMethodRefEntry`.
Thanks!
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 14:45:32 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
>> line 62:
>>
>>> 60: * {@return whether this member is a method}
>>> 61: */
>>> 62: boolean isMethod();
>>
>> this seems surprising - after all we have separate types for methods/fields.
>
> I'll look at it - if still needed or just a relic.
Yes, it is possible to remove this method completely.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:04:22 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
> line 62:
>
>> 60: * {@return whether this member is a method}
>> 61: */
>> 62: boolean isMethod();
>
> this seems surprising - after all we have separate types for methods/fields.
I'll look at it - if still needed or just a relic.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:18:56 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
> line 47:
>
>> 45: * {@return the start of the instruction range}
>> 46: */
>> 47: Label startScope();
>
> I noticed that this pattern of start/endScope is here, but also in
> ExceptionCatch, LocalVariable and LocalVariableType. Consider using a common
> interface for "instructions that are associated with a range".
That is interesting RFE, thanks.
> src/java.base/share/classes/jdk/internal/classfile/instruction/InvokeInstruction.java
> line 60:
>
>> 58: * {@return whether the class holding the method is an interface}
>> 59: */
>> 60: boolean isInterface();
>
> This can also be tested with pattern matching on the MemberRefEntry?
Unfortunately the distinction is dynamic. As of my imagination pattern matching
is not the best to model interface distinction of `InvokeInstruction`.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:24:58 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> AttributeElement.Kind removal (#48) > > src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java > line 63: > >> 61: * aload_0}). >> 62: */ >> 63: sealed interface IntrinsicConstantInstruction extends >> ConstantInstruction > > I'm not super sure of the fine-grained distinction here. The constant pool > variant is interesting (as you can ask for the associated constant entry) - > but the distinction between intrinsics vs. argument seems rather weak. They significantly differ in instruction formats and instruction format distinction is critical for some use cases. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 15:07:01 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/TypeCheckInstruction.java
> line 39:
>
>> 37:
>> 38: /**
>> 39: * Models an {@code instanceof} or {@code checkcast} instruction in the
>> {@code
>
> This seems to model both `instanceof` and `checkcast`. The latter seems to
> overlap partially with `ConvertInstruction`.
`instanceof` and `checkcast` are both very similar type checking instructions.
They have the same length, the same format, the same specification of the
instruction parameters and almost the same behaviour.
All members of `ConvertInstruction` have no instruction parameters and source
and target of conversion is identified from the opcode. Also the instructions
do conversions and not type checking.
I don't see any averlap.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 13:17:30 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 66:
>
>> 64: * @param the type of the optional value
>> 65: */
>> 66: public sealed interface Option permits Options.OptionValue {
>
> After looking more at the usages of Options it is not clear to me as to why
> generics are needed. Lookup is always performed using a non-generic
> Option.Key - so the API code has to be unchecked anyway. In fact, I'm not
> even sure you need a `value()` method. When looking at usages, Option is
> mostly something you create when you need to pass it to something else (e.g.
> a constant pool, etc.). Since the client has created the option, it is not
> clear to me that the client has a need to access the option value (which the
> API implementation can access internally by other means).
You are right, I've tried to remove generics from `Option` and `Option::value`
method and it didn't have any impact on any use case we have.
All access to the `Option` value is done through `ClassReader::optionValue` or
`ConstantPoolBuilder::optionValue`, there was no use of `Option::value` at all.
I think it is valuable API cleanup, thanks!
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 13:10:43 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
> line 98:
>
>> 96: T optionValue(Classfile.Option.Key option);
>> 97:
>> 98: boolean canWriteDirect(ConstantPool constantPool);
>
> Missing javadoc in these two methods.
Will fix it, thanks.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
> line 187:
>
>> 185: * {@return A {@link ModuleEntry} describing the module whose name
>> 186: * is encoded in the provided {@linkplain Utf8Entry}}
>> 187: * If a Module entry in the pool already describes this class,
>
> (Here and elsewhere) - Module is capitalized. Either you use a lower case
> name, or you use a capital name, to refer to `ModuleEntry`, or
> `CONSTANT_Module_info` - e.g. a standalone `Module` with capital `M` is not a
> concept in this API. (personally I think lower case is just fine).
Will fix it, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:01:10 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> AttributeElement.Kind removal (#48) > > src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java > line 537: > >> 535: * @param the type of the entry >> 536: */ >> 537: T maybeClone(T entry); > > This feels a more primitive operation than the name suggests. Have you > considered making ConstantPool extend Consumer and call this > "accept" ? I'm not quite sure what exactly do you propose. `ConstantPool` should not accept anything as it is read-only, so "accept" would be confusing. `ConstantPoolBuilder::maybeClone` is rather a `Function`, where the name might be changed to `ConstantPoolBuilder::apply`. However there are so many "accepts" and "applies" across the API, that reducing API verbosity to just these functional terms might significantly decrease readability. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 13:01:57 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
>> line 80:
>>
>>> 78: * Return a List composed by appending the additions to the base
>>> list.
>>> 79: * @param base The base elements for the list, must not include null
>>> 80: * @param additions The ClassEntrys to add to the list, must not
>>> include null
>>
>> Perhaps we should use `{@code}` or {@link}` to surround type names (here and
>> elsewhere). `ClassEntrys` looks particularly odd :-)
>
> It is odd to see what is essentially a list append operation in here. IMHO,
> these helper methods, if needed (I couldn't find uses in the JDK), should
> probably be added to Collections (which probably means in the jdktypes
> package for now) - as I don't see anything really ClassEntry/ClassDesc
> specific about them.
I'll make a note to deeply review javadoc for types and to wrap them, thanks.
As for the List combining methods, they had been proposed, discussed and
approved here:
https://github.com/openjdk/jdk-sandbox/pull/35
Feel free to re-open the discussion on mailing list, maybe we can address them
better now.
However there is no general contract between "entries" and "symbols" yet, so
such methods could be declared generic.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 12:54:42 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> AttributeElement.Kind removal (#48) > > src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPool.java > line 76: > >> 74: * entry >> 75: */ >> 76: BootstrapMethodEntry bootstrapMethodEntry(int index); > > I note some inconsistency in naming - e.g. is `ByIndex` really needed, or a > letfover to distinguish between different access modes (which are no longer > there, it seems) ? Yes, there are low-level use cases requiring to operate by index, for example javap. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Fri, 10 Feb 2023 11:12:25 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/constantpool/AnnotationConstantValueEntry.java >> line 33: >> >>> 31: * which includes the four kinds of primitive constants, and UTF8 >>> constants. >>> 32: */ >>> 33: public sealed interface AnnotationConstantValueEntry extends PoolEntry >> >> Should this extend LoadableConstantEntry (and restrict it more) ? > > `LoadableConstantEntry` and `AnnotationConstantValueEntry` are just partially > overlapping according to the spec. > The biggest difference (and source of confusion) is that `StringEntry` is > `LoadableConstantEntry`, however not `AnnotationConstantValueEntry` and > `Utf8Entry` is `AnnotationConstantValueEntry`, however not > `LoadableConstantEntry`. Ugh - you are right of course! >> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java >> line 60: >> >>> 58: * viewed as an array of (possibly multi-byte) characters. >>> 59: */ >>> 60: int characterRangeStart(); >> >> Not sure if this belongs here - e.g. it seems to me that >> `characterRangeStart` is less useful than the labels. But I'm not super >> expert on how this code element might be used. > > It is character range within the source code, not the bytecode. I see - I was probably confusing myself (I wonder if the method names played some part in that) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 15:01:03 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/MonitorInstruction.java
> line 48:
>
>> 46: * which must be of kind {@link Opcode.Kind#MONITOR}
>> 47: */
>> 48: static MonitorInstruction of(Opcode op) {
>
> There are only two cases here - perhaps also offer factories for monitor
> enter/exit? Or is creating instruction models a rare operation (e.g. because
> when adapting you always also have a CodeBuilder which has the user-friendly
> methods?)
Each of the approaches server different purposes. `CodeBuilder` with
user-friendly methods is my first choice when typing code from scratch, however
in transformations you usually start with a pattern switch and it is a
Classfile API "standard" to find relevant factory methods in each `Instruction`
sub-type.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 14:11:36 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> AttributeElement.Kind removal (#48) > > src/java.base/share/classes/jdk/internal/classfile/constantpool/Utf8Entry.java > line 47: > >> 45: * @param s the string to compare to >> 46: */ >> 47: boolean equalsString(String s); > > Whatif the provided string contains character that are not representable in > Utf8? Should we throw (e.g. `IllegalArgumentException`) ? I would rather expect `false` value returned, however this case is probably not covered by tests. > src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java > line 60: > >> 58: * viewed as an array of (possibly multi-byte) characters. >> 59: */ >> 60: int characterRangeStart(); > > Not sure if this belongs here - e.g. it seems to me that > `characterRangeStart` is less useful than the labels. But I'm not super > expert on how this code element might be used. It is character range within the source code, not the bytecode. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 13:03:46 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> AttributeElement.Kind removal (#48) > > src/java.base/share/classes/jdk/internal/classfile/constantpool/AnnotationConstantValueEntry.java > line 33: > >> 31: * which includes the four kinds of primitive constants, and UTF8 >> constants. >> 32: */ >> 33: public sealed interface AnnotationConstantValueEntry extends PoolEntry > > Should this extend LoadableConstantEntry (and restrict it more) ? `LoadableConstantEntry` and `AnnotationConstantValueEntry` are just partially overlapping according to the spec. The biggest difference (and source of confusion) is that `StringEntry` is `LoadableConstantEntry`, however not `AnnotationConstantValueEntry` and `Utf8Entry` is `AnnotationConstantValueEntry`, however not `LoadableConstantEntry`. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 08:44:41 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/internal/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:
>
> AttributeElement.Kind removal (#48)
src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
line 47:
> 45: * {@return the start of the instruction range}
> 46: */
> 47: Label startScope();
I noticed that this pattern of start/endScope is here, but also in
ExceptionCatch, LocalVariable and LocalVariableType. Consider using a common
interface for "instructions that are associated with a range".
src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
line 60:
> 58: * viewed as an array of (possibly multi-byte) characters.
> 59: */
> 60: int characterRangeStart();
Not sure if this belongs here - e.g. it seems to me that `characterRangeStart`
is less useful than the labels. But I'm not super expert on how this code
element might be used.
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 63:
> 61: * aload_0}).
> 62: */
> 63: sealed interface IntrinsicConstantInstruction extends
> ConstantInstruction
I'm not super sure of the fine-grained distinction here. The constant pool
variant is interesting (as you can ask for the associated constant entry) - but
the distinction between intrinsics vs. argument seems rather weak.
src/java.base/share/classes/jdk/internal/classfile/instruction/InvokeInstruction.java
line 60:
> 58: * {@return whether the class holding the method is an interface}
> 59: */
> 60: boolean isInterface();
This can also be tested with pattern matching on the MemberRefEntry?
src/java.base/share/classes/jdk/internal/classfile/instruction/MonitorInstruction.java
line 48:
> 46: * which must be of kind {@link Opcode.Kind#MONITOR}
> 47: */
> 48: static MonitorInstruction of(Opcode op) {
There are only two cases here - perhaps also offer factories for monitor
enter/exit? Or is creating instruction models a rare operation (e.g. because
when adapting you always also have a CodeBuilder which has the user-friendly
methods?)
src/java.base/share/classes/jdk/internal/classfile/instruction/TypeCheckInstruction.java
line 39:
> 37:
> 38: /**
> 39: * Models an {@code instanceof} or {@code checkcast} instruction in the
> {@code
This seems to model both `instanceof` and `checkcast`. The latter seems to
overlap partially with `ConvertInstruction`.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 12:57:19 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
> line 80:
>
>> 78: * Return a List composed by appending the additions to the base
>> list.
>> 79: * @param base The base elements for the list, must not include null
>> 80: * @param additions The ClassEntrys to add to the list, must not
>> include null
>
> Perhaps we should use `{@code}` or {@link}` to surround type names (here and
> elsewhere). `ClassEntrys` looks particularly odd :-)
It is odd to see what is essentially a list append operation in here. IMHO,
these helper methods, if needed (I couldn't find uses in the JDK), should
probably be added to Collections (which probably means in the jdktypes package
for now) - as I don't see anything really ClassEntry/ClassDesc specific about
them.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 08:44:41 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/internal/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:
>
> AttributeElement.Kind removal (#48)
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 66:
> 64: * @param the type of the optional value
> 65: */
> 66: public sealed interface Option permits Options.OptionValue {
After looking more at the usages of Options it is not clear to me as to why
generics are needed. Lookup is always performed using a non-generic Option.Key
- so the API code has to be unchecked anyway. In fact, I'm not even sure you
need a `value()` method. When looking at usages, Option is mostly something you
create when you need to pass it to something else (e.g. a constant pool, etc.).
Since the client has created the option, it is not clear to me that the client
has a need to access the option value (which the API implementation can access
internally by other means).
src/java.base/share/classes/jdk/internal/classfile/constantpool/AnnotationConstantValueEntry.java
line 33:
> 31: * which includes the four kinds of primitive constants, and UTF8
> constants.
> 32: */
> 33: public sealed interface AnnotationConstantValueEntry extends PoolEntry
Should this extend LoadableConstantEntry (and restrict it more) ?
src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
line 80:
> 78: * Return a List composed by appending the additions to the base list.
> 79: * @param base The base elements for the list, must not include null
> 80: * @param additions The ClassEntrys to add to the list, must not
> include null
Perhaps we should use `{@code}` or {@link}` to surround type names (here and
elsewhere). `ClassEntrys` looks particularly odd :-)
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPool.java
line 76:
> 74: * entry
> 75: */
> 76: BootstrapMethodEntry bootstrapMethodEntry(int index);
I note some inconsistency in naming - e.g. is `ByIndex` really needed, or a
letfover to distinguish between different access modes (which are no longer
there, it seems) ?
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 98:
> 96: T optionValue(Classfile.Option.Key option);
> 97:
> 98: boolean canWriteDirect(ConstantPool constantPool);
Missing javadoc in these two methods.
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 187:
> 185: * {@return A {@link ModuleEntry} describing the module whose name
> 186: * is encoded in the provided {@linkplain Utf8Entry}}
> 187: * If a Module entry in the pool already describes this class,
(Here and elsewhere) - Module is capitalized. Either you use a lower case name,
or you use a capital name, to refer to `ModuleEntry`, or `CONSTANT_Module_info`
- e.g. a standalone `Module` with capital `M` is not a concept in this API.
(personally I think lower case is just fine).
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 537:
> 535: * @param the type of the entry
> 536: */
> 537: T maybeClone(T entry);
This feels a more primitive operation than the name suggests. Have you
considered making ConstantPool extend Consumer and call this
"accept" ?
src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
line 62:
> 60: * {@return whether this member is a method}
> 61: */
> 62: boolean isMethod();
this seems surprising - after all we have separate types for methods/fields.
src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
line 67:
> 65: * {@return whether this member is an interface method}
> 66: */
> 67: boolean isInterface();
I'd prefer to see this to `MethodRefEntry`. But again, there's an
Re: RFR: 8294982: Implementation of Classfile API [v15]
> 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/internal/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: AttributeElement.Kind removal (#48) - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/df08b351..753e6847 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=13-14 Stats: 160 lines in 16 files changed: 0 ins; 89 del; 71 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
