On Sun, 6 Oct 2024 15:23:15 GMT, Markus KARG <d...@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
