On Sat, 17 Aug 2024 12:25:58 GMT, Shaojin Wen <d...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
>> line 113:
>>
>>> 111: || (rightBracket = (descriptor.charAt(1) == ')' ? 1 :
>>> descriptor.lastIndexOf(')'))) <= 0
>>> 112: || (len = descriptor.length() - rightBracket - 1) == 0
>>> 113: || (len != 1 && len !=
>>> ConstantUtils.skipOverFieldSignature(descriptor, rightBracket + 1,
>>> descriptor.length()))
>>
>> Has this be tested against input values like `()A`? I would prefer to fail
>> explicitly with `descriptor.charAt(rightBracket + 1) == 'V'` instead of
>> using `len != 1` and relying on `Wrapper.forBasicType`.
>
> I have tested that `()A` will report an error. If length == 1, rely on
> Wrapper.forBasicType to detect illegal input.
We just exposed an internal exception behavior to public. Might add in
wrapper.forBasicType that this IAE is part of public API.
>> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
>> line 158:
>>
>>> 156: }
>>> 157: if (len == 0) {
>>> 158: len = ConstantUtils.skipOverFieldSignature(descriptor,
>>> cur, end);
>>
>> This can use a simpler version of skip that doesn't validate, like:
>>
>> int start = cur;
>> while (descriptor.charAt(cur) == '[') {
>> cur++;
>> }
>> if (descriptor.charAt(cur++) == 'L') {
>> cur = descriptor.indexOf(';', cur) + 1;
>> }
>> paramTypes[paramIndex++] = ConstantUtils.resolveClassDesc(descriptor, start,
>> cur - start);
>
> Now this version merges the loops, your suggestion doesn't handle paramIndex
> >= 8, and it won't be faster, the current version of skipOverFieldSignature
> performs well enough.
Hmm? I mean this block can be a new method (the cur pointer moving part) and
replace existing skipOver call that does extra validation
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720770934
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720771392
