On Sat, 5 Oct 2024 16:32:39 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. src/java.base/share/classes/java/io/Reader.java line 152: > 150: * reader was being used by a single thread (as is generally the > case). > 151: * Where possible, it is recommended that this class be used in > preference > 152: * to {@code StringReader} as it will be faster under most > implementations. I believe this paragraph is redundant, now that `StringReader` is a somewhat-deprecated API that we aim to migrate away from. We can note these differences in the API notes of `StringReader`. src/java.base/share/classes/java/io/Reader.java line 161: > 159: * {@code read(char[])}, {@code read(char[], int, int)}, > 160: * {@code read(CharBuffer)}, {@code ready()}, {@code skip(long)}, and > 161: * {@code transferTo()} methods all throw {@code IOException}. I believe these 2 paragraphs are all redundant besides the initial `The returned stream is initially open.` sentence: those are duplicating information already available from `Reader::close` specification. Closing `StringReader` is optional but its specification doesn't mention this. I think we don't need to mention if closing is optional, so that one sentence is sufficient. src/java.base/share/classes/java/io/Reader.java line 163: > 161: * {@code transferTo()} methods all throw {@code IOException}. > 162: * > 163: * <p> The {@code markSupported()} method returns {@code true}. Suggestion: * <p>The returned reader supports the {@link #mark mark()} operation. src/java.base/share/classes/java/io/Reader.java line 174: > 172: * @since 24 > 173: */ > 174: public static Reader of(CharSequence c) { Should we give this factory a more specific name so we don't clash in the future? For example, if we add another factory `of(A a)` for interface `A`, then it would be confusing to have an instance of `interface C extends CharSequence, A` to be passed to `of`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789107588 PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789111313 PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789108008 PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1789104510
