On Thu, 20 Nov 2025 15:53:38 GMT, Jorn Vernee <[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/foreign/MemorySegment.java line 1339:
>
>> 1337: * @param charset the charset used to {@linkplain
>> Charset#newDecoder() decode} the
>> 1338: * string bytes
>> 1339: * @param length length to be used for string conversion, in bytes
>
> Please keep the same format
> Suggestion:
>
> * @param length length in bytes of the string to read
Done
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1341:
>
>> 1339: * @param length length to be used for string conversion, in bytes
>> 1340: * @return a Java string constructed from the bytes read from the
>> given starting
>> 1341: * address reading the given length of characters
>
> Suggestion:
>
> * address reading the given length of bytes
Done
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2662:
>
>> 2660: * are {@code < 0}
>> 2661: * @throws IndexOutOfBoundsException if the {@code endIndex} is
>> larger than the length of
>> 2662: * this {@code String} object, or {@code beginIndex} is
>> larger than {@code endIndex}.
>
> There's no `beginIndex` and `endIndex`?
Done
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 157:
>
>> 155:
>> 156: /**
>> 157: * Converts a Java string into a null-terminated C string using the
>> provided charset,
>
> Not null-terminated, right?
It is not, thanks, fixed
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 177:
>
>> 175: * @return a new native segment containing the converted C string
>> 176: * @throws IllegalArgumentException if {@code charset} is not a
>> 177: * {@linkplain StandardCharsets standard charset}
>
> Same here, I don't think we have this limitation.
Done
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 180:
>
>> 178: * @throws IndexOutOfBoundsException if either {@code srcIndex} or
>> {@code numChars} are {@code < 0}
>> 179: * @throws IndexOutOfBoundsException if the {@code endIndex} is
>> larger than the length of
>> 180: * this {@code String} object, or {@code beginIndex} is
>> larger than {@code endIndex}.
>
> There is no 'endIndext'?
Done
> test/jdk/java/foreign/TestStringEncoding.java line 135:
>
>> 133: }
>> 134: }
>> 135:
>
> We need some more tests for the other new methods as well. Also, it would be
> nice to test non-standard charsets.
I added more tests to cover regular and exception cases for the three new
methods. I'm happy to take suggestions on additional test coverage, or if
there's a better location for any of the tests.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549306779
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549306993
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549311600
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549314628
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549315486
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549315650
PR Review Comment: https://git.openjdk.org/jdk/pull/28043#discussion_r2549319466
