RFR: 8341566: Adding factory for non-synchronized CharSequence Reader [v2]
Chen Liang
liach at openjdk.org
Sun Oct 6 16:44:37 UTC 2024
On Sat, 5 Oct 2024 16:45:01 GMT, Markus KARG <duke at 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
More information about the core-libs-dev
mailing list