RFR: 8331724: Refactor j.l.constant implementation to internal package [v3]

Claes Redestad redestad at openjdk.org
Mon May 6 15:21:55 UTC 2024


On Mon, 6 May 2024 14:58:02 GMT, Chen Liang <liach at openjdk.org> wrote:

>> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Rename ofTrusted to ofValidated, remove accidental module-info exports
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 222:
> 
>> 220:         }
>> 221:         if (desc.length() == 1 && desc.charAt(0) == 'V') {
>> 222:             throw new IllegalArgumentException(String.format("not a valid reference type descriptor: %sV", "[".repeat(rank)));
> 
> Suggestion:
> 
>             throw new IllegalArgumentException(String.format("not a valid reference type descriptor: %sV", "[".repeat(netRank)));
> 
> Or should we override this in `PrimitiveClassDescImpl`, which can bypass the rank sum computation?

`currentDepth` must be 0 in this case, so `rank` or `netRank` doesn't matter. Overriding in `PrimitiveClassDescImpl` sounds reasonable, but then perhaps default method should be removed, too, since it would look strange to have the default method be specialized for instance/array types. Sounds like a CSR might be needed(?), so let's do that in a follow up.

> src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 80:
> 
>> 78:             char ch = name.charAt(i);
>> 79:             if (ch == ';' || ch == '[' || ch == '.')
>> 80:                 throw new IllegalArgumentException("Invalid class name: " + name);
> 
> We can check there's no consecutive `..` `//` or trailing `.` or `/` so we can just use the validated string to create a reference class desc.

Sounds reasonable, but I think I have already piled on too much into this PR. Ok to defer to a follow-up?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591165979
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591167360


More information about the core-libs-dev mailing list