On Wed, 11 Oct 2023 07:17:04 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: > > removed obsolete exports from BuildMicrobenchmark.gmk src/java.base/share/classes/java/lang/classfile/package-info.java line 259: > 257: * the classfile building process. > 258: * <p> > 259: * Syntax validation is applied during symbolic descriptors construction. I would drop `syntax` - and just use `Validation` src/java.base/share/classes/java/lang/classfile/package-info.java line 265: > 263: * <p> > 264: * Using symbolic descriptors assures the right serial form selection by > 265: * the ClassFile API library. For example a class name is stored in its > internal I find this comment still very hard to parse. There is a lot of terminology here thrown to the reader (internal form, class descriptor, serial form), and very little explanation of which is which. I get the general idea that, if I use a `ClassDesc`, the API will take care to convert that to the right form, depending on the context - but it feels there's too much text, and too few examples to ground that text. src/java.base/share/classes/java/lang/classfile/package-info.java line 278: > 276: * constant pool entries to avoid any form of syntax validation or > conversion. > 277: * Constant pool entries can be constructed from raw values, with no > additional > 278: * syntactic checks, conversions or validations. In the following > example is `syntactic checks` and `validation` feel the same thing. If there's some validation that is materially different from a syntactic check that is important for the reader to grasp, then spell it out. Otherwise leave it out and remain more general. src/java.base/share/classes/java/lang/classfile/package-info.java line 401: > 399: * > 400: * <h3>Transformation handling of unknown classfile elements</h3> > 401: * Future JDK releases may introduce new classfile elements, not known > at the Maybe this is more direct/punchy? `Custom classfile transformations might be unaware of classfile elements introduced by future JDK releases.` src/java.base/share/classes/java/lang/classfile/package-info.java line 403: > 401: * Future JDK releases may introduce new classfile elements, not known > at the > 402: * development time of a custom transformation code. > 403: * To achieve deterministic stability of transformations running on > future JDK This para seems largely redundant. Perhaps you can merge with the next? E.g. "To achieve deterministic stability, classfile transforms interested ..." ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356662662 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356661342 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356663577 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356666225 PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356667448
