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

[Adam Sotona]

On Thu, 12 Oct 2023 11:11:10 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> 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`

Yes, applied, thanks.

> 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.

I've tried to rephrase the paragraph and simplify it, thanks.

> 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.

Right, fixed, thanks.

> 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.`

More clear, thanks.

> 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 ..."

Yes, merged according to the suggestion.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356989724
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356989504
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356989983
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356990585
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1356991173