On Mon, 2 Oct 2023 08:34:19 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> Classfile API is an internal library under package `jdk.internal.classfile` >> in JDK 21. >> This pull request turns the Classfile API into a preview feature and moves >> it into `java.lang.classfile`. >> It repackages all uses across JDK and tests and adds lots of missing Javadoc. >> >> This PR goes in sync with >> [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API >> (Preview) (CSR) >> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File >> API (Preview) (JEP). >> >> Online javadoc is available at: >> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html >> >> In addition to the primary transition to preview, this pull request also >> includes: >> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). >> - A new preview feature, `CLASSFILE_API`, has been added. >> - Buildsystem tool required a little patch to support annotated modules. >> - All JDK modules using the Classfile API are newly participating in the >> preview. >> - All tests that use the Classfile API now have preview enabled. >> - A few Javac tests not allowing preview have been partially reverted; their >> conversion can be re-applied when the Classfile API leaves preview mode. >> >> Despite the number of affected files, this pull request is relatively >> straight-forward. The preview version of the Classfile API is based on the >> internal version of the library from the JDK master branch, and there are no >> API features added. >> >> Please review this pull request to help the Classfile API turn into a >> preview. >> >> Any comments are welcome. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > fixed javadoc typo src/java.base/share/classes/java/lang/classfile/constantpool/ConstantPoolBuilder.java line 158: > 156: */ > 157: default ClassEntry classEntry(ClassDesc classDesc) { > 158: if (requireNonNull(classDesc).isPrimitive()) { It is not clear to me as to whether all the methods in the API deal with null correctly. What we did for FFM, assuming that the classfile API is also null-hostile by default, was to add a single test which would try to call all API methods and looking for NPEs: https://github.com/openjdk/jdk/blob/2637e8ddc4ffe102418139f501fc0be8e9c5317b/test/jdk/java/foreign/TestNulls.java#L80 This test has saved us many times when adding/changing API methods. src/java.base/share/classes/java/lang/classfile/instruction/CharacterRange.java line 77: > 75: * A flags word, indicating the kind of range. Multiple flag bits > 76: * may be set. Valid flags include > 77: * {@link java.lang.classfile.ClassFile#CRT_STATEMENT}, Maybe it would be better to organize the links in a bullet list? src/java.base/share/classes/java/lang/classfile/instruction/InvokeInstruction.java line 67: > 65: > 66: /** > 67: * {@return for an {@code invokeinterface}, the {@code count} value, > as defined in {@jvms 6.5}} Suggestion: * {@return the {@code count} value of an {@code invokeinterface} instruction, as defined in {@jvms 6.5}} src/java.base/share/classes/java/lang/classfile/package-info.java line 254: > 252: * No consistency checks are performed while building or transforming > classfiles > 253: * (except for null arguments checks). All builders and classfile > elements factory > 254: * methods accepts provided information without implicit validation. Suggestion: * methods accepts the provided information without implicit validation. src/java.base/share/classes/java/lang/classfile/package-info.java line 255: > 253: * (except for null arguments checks). All builders and classfile > elements factory > 254: * methods accepts provided information without implicit validation. > 255: * However fatal inconsistencies (like for example invalid code sequence > or Suggestion: * However, fatal inconsistencies (like for example invalid code sequence or src/java.base/share/classes/java/lang/classfile/package-info.java line 259: > 257: * the classfile building process. > 258: * <p> > 259: * Basic syntax control can be achieved using symbolic descriptors. For > example What does "syntax control" mean? The concept is used, but not defined, in the javadoc. (I presume we mean eg.g. turning String into `Ljava/lang/String;`) src/java.base/share/classes/java/lang/classfile/package-info.java line 266: > 264: * On the other hand it is possible to use builders methods and > factories accepting > 265: * constant pool entries to avoid any form of syntax control. Constant > pool entries > 266: * can be constructed from raw values, where no syntax control is in > charge. Suggestion: * can be constructed from raw values, with no additional syntactic checks. src/java.base/share/classes/java/lang/classfile/package-info.java line 273: > 271: * <p> > 272: * More complex verification of a classfile can be achieved by explicit > invocation > 273: * of {@link java.lang.classfile.ClassModel#verify}. Aren't part of verification also ran as part of generating stackmaps (unless stackmap inference is disabled) ? Should it be mentioned here? src/java.base/share/classes/java/lang/classfile/package-info.java line 386: > 384: * resulting in many unreferenced constant pool entries. > 385: * > 386: * <h3>Transformation handling of unknown classfile elements from a > future</h3> >From a future... ? JDK? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342714653 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342716987 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342720961 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342724684 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342725264 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342726203 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342728341 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342730435 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342730871
