On Sun, 6 Oct 2024 13:14:32 GMT, Alan Bateman <al...@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. > > Reader.of(CharSequence) looks much better than introducing > CharSequenceReader. It won't have an equivalent on Writer but I think that is > okay. Also it means that the user will need to deal with close throwing > IOException but anything using Reader has to do this already. > > I think it would be better to drop "API compatible with StringReader" from > the method description. An apiNote in StringReader can direct readers to the > static factory method. > > Also I think drop the "lock" field from the API docs as it's a protected > field and only interesting to subclasses. The Reader class does not specify > if Reader is thread-safe so the method description doesn't need to say too > much. For clarify then it could just say something like "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". > > The parameter name is currently "c", maybe you mean "cs"? The method will > need to specify NPE for the of(null) case. > @AlanBateman Can you please review [the CSR > request](https://bugs.openjdk.org/browse/JDK-8341596) so I can finish it? > Thanks! Latest API docs looks good, will you update the CSR? ------------- PR Comment: https://git.openjdk.org/jdk/pull/21371#issuecomment-2401489513
