On Wed, 9 Oct 2024 11:49:47 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 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
