On Thu, 16 Feb 2023 10:41:33 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: > > 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
