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/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
