On Wed, 14 Jan 2026 22:49:06 GMT, Roger Riggs <[email protected]> wrote:
>> Liam Miller-Cushon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review feedback
>
> src/java.base/share/classes/java/lang/String.java line 1080:
>
>> 1078: return value.length;
>> 1079: }
>> 1080: int len = value.length >> 1;
>
> I don't think I understand what's being done and what Charset encoder it is
> mimicking.
> It probably needs to document the assumptions about unmappable characters and
> malformed surrogates.
> (Likely it is correct since the test of US_ASCII passes, but could use an
> explanation).
I added some `//` comments documenting which methods the `encodedLength*`
methods are mimicking. The logic here should be identical to `encodeASCII`
(except that it isn't allocating and writing to a destination array).
The handling of unmappable characters and malformed surrogates should match
`encodeASCII`.
> src/java.base/share/classes/java/lang/String.java line 2130:
>
>> 2128:
>> 2129: /**
>> 2130: * Returns the length in bytes of the given String encoded with
>> the given {@link Charset}.
>
> You can use the javadoc tag `@return` and skip the duplication. This first
> sentence reads better then the @return below since it emphasies the "encoded
> string" aspect.
Sorry I'm not sure I understand, can you clarify how that would work?
The javadoc can't start with `@return`, it needs to be a non-tag sentence
fragment (the build enables doclint to enforce this).
> src/java.base/share/classes/java/lang/String.java line 2136:
>
>> 2134: * @implNote This method may allocate memory to compute the length
>> for some charsets.
>> 2135: *
>> 2136: * @param cs the charset used to the compute the length
>
> Capitalize and perhaps link "Charset".
Done
> src/java.base/share/classes/java/lang/String.java line 2155:
>
>> 2153: }
>> 2154:
>> 2155: private int byteFor(int length, int coder) {
>
> Add a //comment please.
> The method name should be plural. `bytesForCoder`.
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28454#discussion_r2693532105
PR Review Comment: https://git.openjdk.org/jdk/pull/28454#discussion_r2693479863
PR Review Comment: https://git.openjdk.org/jdk/pull/28454#discussion_r2693480259
PR Review Comment: https://git.openjdk.org/jdk/pull/28454#discussion_r2693532859
