On Sat, 12 Oct 2024 07:47:56 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: > > Revert 'inc should be faster than add on most CPUs' The latest version of this PR (commit `487b9386`) looks OK to me. Thank you for the updates related to the review comments. I don't have anything more. My only other comment was test related - to additionally test the returned `Reader`'s APIs against corner cases like an empty `CharSequence` and a `CharSequence` with `Integer.MAX_VALUE` length. However, if you have run out of time and energy on this one, then the test is fine in the current form. I don't have the necessary bandwidth to propose a test snippet for it right now and we can enhance the test separately at a later date. test/jdk/java/io/Reader/Of.java line 92: > 90: > 91: @Test(dataProvider = "readers") > 92: public void testReadBII(Reader reader) throws IOException { What does `BII` in some of these test method names represent? ------------- Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21371#pullrequestreview-2363830708 PR Comment: https://git.openjdk.org/jdk/pull/21371#issuecomment-2408455268 PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1797632705
