On Mon, 6 Feb 2023 13:29:10 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 three additional > commits since the last revision: > > - javadoc fixes > - obsolete identifiers and unused imports cleanup > - TypeAnnotation.TypePathComponent cleanup src/java.base/share/classes/jdk/internal/classfile/attribute/CharacterRangeInfo.java line 80: > 78: * well as increment and decrement expressions (both prefix and > postfix). > 79: * <li>{@link jdk.internal.classfile.Classfile#CRT_FLOW_CONTROLLER} > An expression > 80: * whose value will effect control flow. Flowcon in the following: This sentence seems hard to parse. If `Flowcon` is the name of an expression in pseudocode, perhaps using `{@code}` would help. src/java.base/share/classes/jdk/internal/classfile/attribute/CharacterRangeInfo.java line 146: > 144: * @param flags the range flags > 145: */ > 146: static CharacterRangeInfo of(int startPc, I get that the encoding is as per JaCoCo specification. However, I also wonder if the API should provide better accessors and or factories to let clients just reason about line/columns numbers. Or, maybe, provide static helpers to do the encoding/decoding. src/java.base/share/classes/jdk/internal/classfile/attribute/CodeAttribute.java line 52: > 50: byte[] codeArray(); > 51: > 52: int labelToBci(Label label); Missing javadoc here. (also, this method looks a bit odd in here - but I see uses e.g. in ClassPrinterImpl) src/java.base/share/classes/jdk/internal/classfile/attribute/ModuleAttribute.java line 132: > 130: } > 131: > 132: static ModuleAttribute of(ModuleDesc moduleName, Some missing javadoc in this class src/java.base/share/classes/jdk/internal/classfile/attribute/ModuleExportInfo.java line 107: > 105: * @param exportsTo the modules to which this package is exported > 106: */ > 107: static ModuleExportInfo of(PackageEntry exports, (Here and elsewhere) - should there be factories also taking a PackageDesc (which seems to be an extension of the ClassDesc API, but for packages) ? src/java.base/share/classes/jdk/internal/classfile/attribute/NestMembersAttribute.java line 72: > 70: * @param nestMembers the member classes of the nest > 71: */ > 72: static NestMembersAttribute ofSymbols(List<ClassDesc> nestMembers) { I see that in some classes we use the `Symbols` prefix, in others we just use `of` - we should try to make the more consistent (in the future). src/java.base/share/classes/jdk/internal/classfile/attribute/StackMapTableAttribute.java line 68: > 66: * A simple stack value. > 67: */ > 68: public enum SimpleVerificationTypeInfo implements > VerificationTypeInfo { I note that in this class we have made the decision to model all the innards (XYZInfo) as nested classes - while in all the other cases XYZInfo are toplevel types. Moving forward, we should pick something consistent. ------------- PR: https://git.openjdk.org/jdk/pull/10982
