On Mon, 24 Apr 2023 13:18:09 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 one additional
> commit since the last revision:
>
> added links to JVMS and utility methods moved to ConstantUtils
src/java.base/share/classes/java/lang/constant/ConstantUtils.java line 80:
> 78: /**
> 79: * Validates the correctness of a binary package name. In particular
> checks for the presence of
> 80: * invalid characters in the name.
It may be useful to note that the empty string are considered valid.
Also add @throws NPE; there's an implicit null check in checking the length.
Also in validateBinaryPackageName and validateModuleName.
src/java.base/share/classes/java/lang/constant/ConstantUtils.java line 130:
> 128: }
> 129: return name;
> 130: }
If these are performance sensitive or get used a lot, should there be an array
of flags indexed by the byte/char to indicate the valid cases?
src/java.base/share/classes/java/lang/constant/ModuleDescImpl.java line 27:
> 25: package java.lang.constant;
> 26:
> 27: record ModuleDescImpl(String moduleName) implements ModuleDesc {
Given the validation is elsewhere, it might be useful to add a comment saying
the moduleName must have been validated.
src/java.base/share/classes/java/lang/constant/package-info.java line 92:
> 90: * reading and writing APIs.
> 91: *
> 92: * <p>Another members of this package are {@link
> java.lang.constant.ModuleDesc}
"Another" -> "Other"
test/jdk/java/lang/constant/ModuleDescTest.java line 37:
> 35:
> 36: class ModuleDescTest {
> 37:
Add tests for empty and null arguments.
test/jdk/java/lang/constant/PackageDescTest.java line 39:
> 37:
> 38: class PackageDescTest {
> 39: @ParameterizedTest
Add tests for empty and null arguments.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175375915
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175366692
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175371637
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175382854
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175383972
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175384822
