RFR: 8341566: Adding factory for non-synchronized CharSequence Reader [v2]

[Chen Liang]

On Sun, 6 Oct 2024 15:23:15 GMT, Markus KARG <duke at openjdk.org> wrote:

>> This Pull Requests proposes an implementation for [JDK-8341566](https://bugs.openjdk.org/browse/JDK-8341566): Adding the new method `public static Reader Reader.of(CharSequence)` will return an anonymous, non-synchronized implementation of a `Reader` for each kind of `CharSequence` implementation. It is optimized for `String`, `StringBuilder`, `StringBuffer` and `CharBuffer`.
>> 
>> In addition, this Pull Request proposes to replace the implementation of `StringReader` to become a simple synchronized wrapper around `Reader.of(CharSequence)` for the case of `String` sources. To ensure correctness, this PR...
>> * ...simply moved the **original code** of `StringBuilder` to become the de-facto implementation of `Reader.of()`, then stripped synchronized from it on the left hand, but kept just a synchronized wrapper on the right hand. Then added a `switch` for optimizations within the original code, at the exact location where previously just an optimization for `String` lived in.
>> * ...added tests for all methods (`Of.java`), and applied that test upon the modified `StringBuilder`.
>> 
>> Wherever new JavaDocs were added, existing phrases from other code locations have been copied and adapted, to best match the same wording.
>
> 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.

src/java.base/share/classes/java/io/Reader.java line 174:

> 172:         return new Reader() {
> 173:             private final int length = source.length();
> 174:             private CharSequence cs = source;

Can we make this `cs` final and track the closed state either in a new boolean field or with a negative value in `next`? In fact, if we make `cs` final, we can just replace the `cs` field referenced with the captured `source`.

src/java.base/share/classes/java/io/Reader.java line 187:

> 185:              * Reads a single character.
> 186:              *
> 187:              * @return     The character read, or -1 if the end of the stream has been

Don't need these javadocs; this implementation class is not publicly visible.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789160758
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789160971