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/ClassTransform.java line 164:

> 162: 
> 163:     /**
> 164:      * @implSpec

Should these overridden method have `@inheritDoc` ? If not, their javadoc won't 
show much, I believe. (same might be true for other methods)

src/java.base/share/classes/java/lang/classfile/CodeBuilder.java line 596:

> 594:      * @param type the object type
> 595:      * @return this builder
> 596:      * @throws IllegalArgumentException if {@code type} represents a 
> primitive type

What about arrays? (ClassDesc has an `isArray` predicate too). This probably 
applies to other methods too.

src/java.base/share/classes/java/lang/classfile/CodeTransform.java line 101:

> 99: 
> 100:     /**
> 101:      * @implSpec The default implementation returns a resolved transform;

`... returns a resolved transform that is bound to the given code builder`
or
`... returns a resolved transform bound to the given code builder`

src/java.base/share/classes/java/lang/classfile/FieldTransform.java line 116:

> 114: 
> 115:     /**
> 116:      * @implSpec The default implementation returns a resolved transform 
> with all

Perhaps lose the `all its parts` - not sure that makes the comment any clearer?

src/java.base/share/classes/java/lang/classfile/MethodSignature.java line 59:

> 57: 
> 58:     /**
> 59:      * {@return method signature for a raw (no generic information) 
> method descriptor}

Isn't this missing an article - e.g. `return the method signature` or `return a 
method signature` ?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342685585
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342691511
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342693515
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342694834
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342696367

Reply via email to