On Thu, 9 Feb 2023 08:44:41 GMT, Adam Sotona <asot...@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
