On Mon, 24 Apr 2023 14:30:46 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> 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.
I'll add that notes, thanks.
> 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?
It is just a range check and three escaped characters check. The range check
can be implemented bit-wise and escaped check may be by a switch case, however
I don't think there would be any performance benefits.
> 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.
I'll add it, thanks.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175483446
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175482952
PR Review Comment: https://git.openjdk.org/jdk/pull/13615#discussion_r1175484294
