RFR: 8308753: Class-File API transition to Preview [v19]

[Maurizio Cimadamore]

On Wed, 11 Oct 2023 07:17:04 GMT, Adam Sotona <asotona at 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