On Fri, 3 Feb 2023 14:31:24 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/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<VerifyError> verify(Consumer<String> 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<BlockCodeBuilder> handler) { > 201: Label breakLabel = newLabel(); > 202: BlockCodeBuilderImpl child = new BlockCodeBuilderImpl(this, > breakLabel); Nice trick! I like the `breakLabel` concept src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 333: > 331: * @see #catchingAll > 332: */ > 333: CatchBuilder catching(ClassDesc exceptionType, > Consumer<BlockCodeBuilder> 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. 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` ? src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 1371: > 1369: } > 1370: > 1371: default CodeBuilder tableswitch(Label defaultTarget, > List<SwitchCase> 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. src/java.base/share/classes/jdk/internal/classfile/FieldTransform.java line 92: > 90: * @return the field transform > 91: */ > 92: static FieldTransform dropping(Predicate<FieldElement> 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). 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... 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? 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 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. 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`? 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 src/java.base/share/classes/jdk/internal/classfile/package-info.java line 39: > 37: * <p> > 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` ------------- PR: https://git.openjdk.org/jdk/pull/10982
