RFR: 8294982: Implementation of Classfile API [v15]

[Maurizio Cimadamore]

On Thu, 9 Feb 2023 08:44:41 GMT, Adam Sotona <asotona at openjdk.org> 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 <V> the type of the optional value
> 65:      */
> 66:     public sealed interface Option<V> 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> 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 <T> the type of the entry
> 536:      */
> 537:     <T extends PoolEntry> T maybeClone(T entry);

This feels a more primitive operation than the name suggests. Have you considered making ConstantPool extend Consumer<PoolEntry> 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 entry for interface methods, so not sure how much this is needed.

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.

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` ?

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`) ?

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

PR: https://git.openjdk.org/jdk/pull/10982