RFR: 8294982: Implementation of Classfile API [v20]

[Maurizio Cimadamore]

On Thu, 16 Feb 2023 10:41:33 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:
> 
>   added 4-byte Unicode text to Utf8EntryTest

src/java.base/share/classes/jdk/internal/classfile/components/ClassPrinter.java line 93:

> 91:          * @return name of the node
> 92:          */
> 93:         public ConstantDesc name();

Not sure about the ConstantDesc here. Why is it better than String?

src/java.base/share/classes/jdk/internal/classfile/components/ClassPrinter.java line 105:

> 103:          * @param out consumer of the printed fragments
> 104:          */
> 105:         default public void toJson(Consumer<String> out) {

there are some `public` modifiers here which can be omitted

src/java.base/share/classes/jdk/internal/classfile/components/ClassPrinter.java line 140:

> 138: 
> 139:     /**
> 140:      * A tree node holding {@link List} of nested nodes.

It would perhaps be beneficial to have little examples of what these different nodes are used for. I had to look at `ClassPrinterImpl` to get some idea.

src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java line 93:

> 91: 
> 92: /**
> 93:  * ClassRemapper is a {@link jdk.internal.classfile.ClassTransform}, {@link jdk.internal.classfile.FieldTransform},

Maybe wrap occurrences of `ClassRemapper` with `{@code}` (here and elsewhere)

src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java line 168:

> 166:         public void accept(ClassBuilder clb, ClassElement cle) {
> 167:             switch (cle) {
> 168:                 case FieldModel fm ->

What about NestMembers, NestHost, PermittedSubclasses (and probably others) ?

src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java line 306:

> 304: 
> 305:         ClassSignature mapClassSignature(ClassSignature signature) {
> 306:             return ClassSignature.of(signature.typeParameters(),

Should type parameters also be mapped? (as they might have class bounds). Both here and in `mapMethodSignature`.

src/java.base/share/classes/jdk/internal/classfile/components/CodeLocalsShifter.java line 43:

> 41: 
> 42: /**
> 43:  * CodeLocalsShifter is a {@link jdk.internal.classfile.CodeTransform} shifting locals to

Missing `{@code}` (here and elsewhere)

src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java line 43:

> 41: 
> 42: /**
> 43:  * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform} replacing all occurences

Suggestion:

 * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform} replacing all occurrences

src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java line 43:

> 41: 
> 42: /**
> 43:  * CodeStackTracker is a {@link jdk.internal.classfile.CodeTransform} synchronously tracking

Not sure what is meant by `synchronously`

src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java line 53:

> 51:  *         trackedBuilder.aload(0);
> 52:  *         trackedBuilder.lconst_0();
> 53:  *         trackedBuilder.ifThen(...

missing closed parens?

src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java line 77:

> 75:       *
> 76:       * Temporary unknown stack content can be recovered by binding of a {@linkplain Label} used as
> 77:       * target of a branch instruction from existing code with known Stack (forward branch target),

Suggestion:

      * target of a branch instruction from existing code with known stack (forward branch target),

src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java line 86:

> 84:     /**
> 85:       * Returns tracked max stack size.
> 86:       * Returns an empty {@linkplain Optional} when Max stack size tracking has been lost.

Suggestion:

      * Returns an empty {@linkplain Optional} when max stack size tracking has been lost.

src/java.base/share/classes/jdk/internal/classfile/components/package-info.java line 97:

> 95:  * {@snippet lang="java" class="PackageSnippets" region="classInstrumentation"}
> 96:  */
> 97: package jdk.internal.classfile.components;

watch out for newline

src/java.base/share/classes/jdk/internal/classfile/impl/ClassPrinterImpl.java line 563:

> 561:                       list("attributes", "attribute", clm.attributes().stream().map(Attribute::attributeName)))
> 562:                 .with(constantPoolToTree(clm.constantPool(), verbosity))
> 563:                 .with(attributesToTree(clm.attributes(), verbosity))

Is this ok? It seems we also add class attributes in the map node (see "attributes" map node).

src/java.base/share/classes/jdk/internal/classfile/impl/ClassPrinterImpl.java line 745:

> 743:                     list("stack", "item", Stream.of())));
> 744:             }
> 745:             var excHandlers = com.exceptionHandlers().stream().map(exc -> new ExceptionHandler(com.labelToBci(exc.tryStart()), com.labelToBci(exc.tryEnd()), com.labelToBci(exc.handler()), exc.catchType().map(ct -> ct.name().stringValue()).orElse(null))).toList();

Watch out for very long lines

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

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