On Mon, 6 May 2024 14:58:02 GMT, Chen Liang <li...@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