On Tue, 25 Apr 2023 08:11:24 GMT, Adam Sotona <[email protected]> wrote:
>> Constants API already provides models for all loadable constants to help
>> programs manipulating class files and modelling bytecode instructions.
>> However no models of module and package constants are provided by Constants
>> API. Every program manipulating class files must implement own models and
>> validation of modules and packages constants.
>>
>> This pul request adds `java.lang.constant.ModuleDesc` and
>> `java.lang.constant.PackageDesc` to the Constants API.
>>
>> Classfile API will follow up and remove its internal implementations of
>> `PackageDesc` and `ModuleDesc`.
>>
>> Please review this pull request and attached CSR.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> added custom toString() methods
A side note: I notice that the type names are linked with `@linkplain` in this
package mixed with some `@link`. I'm not sure if it's intentional as type
names are generally used `@link` or `@code`.
src/java.base/share/classes/java/lang/constant/ModuleDesc.java line 64:
> 62: * an at-sign in a module name.
> 63: * </ul>
> 64: * @param name module name
Suggestion:
* @param name the module name
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 33:
> 31: *
> 32: * <p>To create a {@linkplain PackageDesc} for a package, use {@link #of}
> or
> 33: * {@link #ofInternalName(String)}.
Suggestion:
* <p>To create a {@linkplain PackageDesc} for a package, use the {@link #of} or
* {@link #ofInternalName(String)} method.
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 44:
> 42: * given the name of the package, such as {@code "java.lang"}.
> 43: * <p>
> 44: * {@jls 13.1}
Do you mean to reference JLS 6.7?
Suggest to move this reference after `throws` in the see also section.
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 46:
> 44: * {@jls 13.1}
> 45: *
> 46: * @param name the fully qualified (dot-separated) binary package name
JLS 13.1 does not describe a package has a binary name. Is it correct to say
"binary package name"?
Suggestion:
* @param name the fully qualified (dot-separated) package name
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 62:
> 60: * such as {@code "java/lang"}.
> 61: * <p>
> 62: * {@jvms 4.2.1} In this internal form, the ASCII periods (.) that
> normally
Suggest not to copy JVMS 4.2.1 here but instead just add it to see also section.
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 65:
> 63: * separate the identifiers
> 64: * which make up the binary name are replaced by ASCII forward
> slashes (/).
> 65: * @param name the fully qualified class name, in internal
> (slash-separated)
Suggestion:
* @param name the fully qualified package name, in internal
(slash-separated) form
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 82:
> 80: *
> 81: * @return the package name, or the empty string for the
> 82: * default package
Suggestion:
* unnamed package
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 91:
> 89: *
> 90: * @return the package name, or the empty string for the
> 91: * default package
Suggestion:
* unnamed package
See JLS 7.4.2
src/java.base/share/classes/java/lang/constant/package-info.java line 93:
> 91: *
> 92: * <p>Other members of this package are {@link
> java.lang.constant.ModuleDesc}
> 93: * and {@link java.lang.constant.PackageDesc}. They represent module and
> package
Suggestion:
* <p>Other members of this package are {@link ModuleDesc}
* and {@link PackageDesc}. They represent module and package
same package and so no need to qualify.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13615#pullrequestreview-1398780011
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176758742
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176759664
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176763362
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176770337
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176781861
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176768935
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176789226
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176787981
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1176790549
