RFR: 8341566: Add Reader.of(CharSequence) [v7]

[Roger Riggs]

On Wed, 9 Oct 2024 11:49:47 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 two additional commits since the last revision:
> 
>  - assertThrows instead of expectedExceptions
>  - ordered jtreg tags according recommendation in https://openjdk.org/jtreg/tag-spec.html#ORDER

Looks pretty good; some suggestions about clarity of the javadoc.
Claims are made about performance but are minimal and unsubstantiated with JMH tests.
Some updates to the CSR will be needed after updating the javadoc.

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

> 143:     /**
> 144:      * Returns a {@code Reader} that reads characters from a
> 145:      * {@code CharSequence}, starting at the first character in the sequence.

The first line comment should be brief and avoid unnecessary details. It is used in method summaries and should stand alone.
The phrase ", starting at the first character in the sequence" should be moved to a separate sentence.

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

> 159:      * {@code transferTo()} methods all throw {@code IOException}.
> 160:      *
> 161:      * <p> The returned reader supports the {@link #mark mark()} operation.

It may be useful to mention and link to the #reset method as well.

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

> 166:      *
> 167:      * @throws NullPointerException if {@code cs} is {@code null}
> 168:      *

Drop the extra empty lines, to be more consistent with Reader code style and make the source more compact.

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

> 200:                 if (next >= cs.length())
> 201:                     return -1;
> 202:                 int n = Math.min(cs.length() - next, len);

Is there any sensitivity to cs.length() changing between these calls? (For an arbitrary custom implementation).
The spec for undefined behavior covers that but might be safer to compute a local length.

src/java.base/share/classes/java/io/StringReader.java line 38:

> 36:  * {@code Reader.of(String)} should generally be used in preference to this one,
> 37:  * as it supports all of the same operations but it is faster, as it performs no
> 38:  * synchronization.

Consider how this API note will age gracefully in a few releases.
I'd lead with the link to the new method and mention it is unsynchronized/single-thread safe.
The comments about performance and "better" belong in the release note.
Uncontested locks will be barely noticeable from a performance perspective.

test/jdk/java/io/Reader/Of.java line 40:

> 38:  * @bug 8341566
> 39:  * @summary Check for expected behavior of Reader.of().
> 40:  * @run testng Of

The test name should be more descriptive.  Without the package name it is unknown what this test does or what it tests.

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

PR Review: https://git.openjdk.org/jdk/pull/21371#pullrequestreview-2357911836
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793987936
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793997115
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1794003619
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1794026096
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1794048328
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1794058509