On Mon, 10 Aug 2026 09:16:18 GMT, Volkan Yazici <[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 2179:
>
>> 2177: }
>> 2178:
>> 2179: // This method is intended to be used together with
>> bytesCompatible.
>
> Can we make this a Javadoc with a valid link to the `bytesCompatible` method,
> please?
>
> This will make IDE-assisted jumping/linking in between two easier.
Done
> src/java.base/share/classes/java/lang/String.java line 2180:
>
>> 2178:
>> 2179: // This method is intended to be used together with
>> bytesCompatible.
>> 2180: int copyToSegmentRaw(MemorySegment segment, long offset, int
>> srcIndex, int srcLength) {
>
> Not a direct goal of this PR, but since you're substantially changing this
> method anyway... Shouldn't this method be checking its arguments?
Done. (The callers of there internal methods should all be validating these
parameters, but from looking at other similar internal helpers they mostly
repeat argument checking.)
> test/jdk/java/foreign/TestStringEncoding.java line 612:
>
>> 610: for (Charset charset : standardCharsets()) {
>> 611: boolean expected = compatibleCharsets.contains(charset);
>> 612: boolean actual = StringSupport.bytesCompatible(string,
>> charset, 0, string.length());
>
> Shall we also exercise non-zero `srcIndex`? Consider split surrogate pairs.
> (This can also be a separate test with only this concern.)
Thanks, I added more test coverage for split surrogate pairs
> test/jdk/java/foreign/TestStringEncoding.java line 613:
>
>> 611: boolean expected = compatibleCharsets.contains(charset);
>> 612: boolean actual = StringSupport.bytesCompatible(string,
>> charset, 0, string.length());
>> 613: assertEquals(actual, expected);
>
> On failures, the detail will be very coarse. That is, I will get an
> `AssertionFailure` for the input, say,
>
> {
> List.of("", "hello world", "123"),
> Set.of(
> StandardCharsets.US_ASCII,
> StandardCharsets.ISO_8859_1,
> StandardCharsets.UTF_8),
> }
>
> But it won't be clear which case has failed. `"123" & US_ASCII`? `"hello
> world" & UTF_8`? What do you think about flattening the output of
> `stringsAndCompatibleCharsets`? Or adding some detail message to
> `assertEquals`?
I took a pass over the `assert*` calls here and added detail messages.
> test/jdk/java/foreign/TestStringEncoding.java line 627:
>
>> 625: String substring =
>> string.substring(srcIndex, srcIndex + numChars);
>> 626: var segment =
>> arena.allocate(substring.encodedLength(charset));
>> 627: StringSupport.copyToSegmentRaw(string,
>> segment, 0, srcIndex, numChars);
>
> Shall we also exercise non-zero `offset`? (This can also be a separate test
> with only this concern.)
Done, I added another test method for non-zero `offset`
> test/jdk/java/foreign/TestStringEncoding.java line 628:
>
>> 626: var segment =
>> arena.allocate(substring.encodedLength(charset));
>> 627: StringSupport.copyToSegmentRaw(string,
>> segment, 0, srcIndex, numChars);
>> 628: assertEquals(segment.toArray(JAVA_BYTE),
>> substring.getBytes(charset));
>
> What do you think about adding some detail message to `assertEquals`?
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3751449875
PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3751461218
PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3751467116
PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3751464004
PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3751469735
PR Review Comment: https://git.openjdk.org/jdk/pull/32268#discussion_r3751464757
