On Sat, 5 Oct 2024 16:45:01 GMT, Markus KARG <d...@openjdk.org> wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixup! Reader.of(String)
>>
>> Updated JavaDocs according to Alan Bateman's review comments:
>> * Dropped "API compatible with StringReader" from description
>> * @apiNote in StringReader refers to static factory method
>> * Dropped "lock" field from API docs
>> * Added "The resulting Reader is not safe for use by multiple concurrent
>> threads. If the Reader is to be used by more than one thread it should be
>> controlled by appropriate synchronization"
>> * Deviating from your proposal, I renamed parameter "c" to "source" for
>> clarity as the name "cs" already exists as an internal variable
>> * Method specifies NPE for `of(null)` case
>>
>> I addition, JavaDocs now says "reader", not "stream" anymore.
>
> test/jdk/java/io/Reader/Of.java line 51:
>
>> 49: public static Reader[] readers() {
>> 50: return new Reader[] {
>> 51: new StringReader(CONTENT),
>
> Explicitly including that original class here (even if it has nothing to do
> with the `of` method) to be sure that we did not modify it in an incompatible
> way. Unfortunately there is no full test coverage for `StringReader`, and it
> does not make much sense to duplicate the tests.
I recommend adding another test case against an ad-hoc `CharSequence`
implementation wrapping a `char[]` in a record, to ensure the generic paths in
`read(char[], int, int)` works as intended.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789161659
