On Tue, 6 May 2025 01:38:21 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refactor to consistently use `isLatin1(coder)` within
>> AbstractStringBuilder.
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 277:
>
>> 275: // copy all bytes to new larger buffer
>> 276: value = Arrays.copyOf(value,
>> 277: newCapacity(value, newCoder, minimumCapacity)
>> << newCoder);
>
> Would this benefit with an `assert` to verify that the new buffer length
> being proposed is not lesser than the current buffer's length (thus
> truncating content). Something like:
>
>
> // copy all bytes to new larger buffer
> int newLen = newCapacity(value, newCoder, minimumCapacity) << newCoder;
> assert newLen >= value.length : "bad new length " + newLen + " for buffer's
> length " + value.length;
> value = Arrays.copyOf(value, newLen);
I'd be concerned about bloating the code size for a check that will not be
triggered.
`growth` is non-negative and `newCapacity()` uses the same comparison to
determine its growth value.
If ArraySupport.newLength can't satisify the request, it will throw.
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 309:
>
>> 307: if (minimumCapacity - oldCapacity > 0) {
>> 308: value = Arrays.copyOf(value,
>> 309: newCapacity(value, coder, minimumCapacity) <<
>> coder);
>
> Same question about whether we should assert that the new length we determine
> for the buffer is not lesser than the current buffer length.
ditto, will not return a shorter array, it will throw instead.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24967#discussion_r2075667207
PR Review Comment: https://git.openjdk.org/jdk/pull/24967#discussion_r2075668360
