On Tue, 25 Apr 2023 19:43:29 GMT, Adam Sotona <asot...@openjdk.org> 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 two additional
> commits since the last revision:
>
> - Update ModuleDesc.java
> - Update PackageDesc.java
I wonder if `packageName()` and `packageInternalName()` methods can simply be
`name()` and `internalName()` as the type name is `PackageDesc` and `package`
prefix seems to be unnecessary. Similarly, `moduleName()` can be `name()`.
Have this be discussed and rejected?
src/java.base/share/classes/java/lang/constant/ModuleDesc.java line 32:
> 30: * A nominal descriptor for a {@code Module} constant.
> 31: *
> 32: * @apiNote
can drop `@apiNote` as it's not needed to be. `<p>` if you want to a new
paragraph.
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 32:
> 30: * A nominal descriptor for a {@code Package} constant.
> 31: *
> 32: * @apiNote
can drop `@apiNote` as it's not needed to be. `<p>` if you want to a new
paragraph.
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 51:
> 49: * @throws IllegalArgumentException if the name string is not in the
> 50: * correct format
> 51: * @jls 6.7 Fully Qualified Names and Canonical Names
Suggestion:
* @jls 6.5.3 Module Names and Package Names
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 70:
> 68: * @throws IllegalArgumentException if the name string is not in the
> 69: * correct format
> 70: * @jvms 4.2.1 Binary Class and Interface Names
Worth adding `@jvm 4.2.3 Module and Package Names`
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 79:
> 77:
> 78: /**
> 79: * Returns the fully qualified (slash-separated) internal package name
Suggestion:
* Returns the fully qualified (slash-separated) package name in internal
form
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 82:
> 80: * of this {@link PackageDesc}.
> 81: *
> 82: * @return the package name, or the empty string for the
Suggestion:
* @return the package name in internal form, or the empty string for the
src/java.base/share/classes/java/lang/constant/PackageDesc.java line 89:
> 87:
> 88: /**
> 89: * Returns the fully qualified (dot-separated) binary package name
Suggestion:
* Returns the fully qualified (dot-separated) package name
-------------
PR Review: https://git.openjdk.org/jdk/pull/13615#pullrequestreview-1400784174
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177094576
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177084404
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177085890
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177088336
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177088970
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177089545
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1177097732
