RFR: 8341566: Adding factory for non-synchronized CharSequence Reader [v6]

[Jaikiran Pai]

On Wed, 9 Oct 2024 05:50:18 GMT, Markus KARG <duke at 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:
> 
>   Improved wording: 'If the sequence changes while the reader is open, e.g. the length changes, the behavior is undefined.'

test/jdk/java/io/Reader/Of.java line 168:

> 166:     }
> 167: 
> 168:     @Test(dataProvider = "readers", expectedExceptions = IOException.class)

Instead of `expectedExceptions` at the method level, I think it would be better to use something like:


org.testng.Assert.assertThrows(IOException.class,
                               () -> {reader.read();});

That way it's clear which operation within the test method is expected to throw this exception and it also prevents unexpected `IOException` (for example, from reader.close()) from going unnoticed.

test/jdk/java/io/Reader/Of.java line 174:

> 172:     }
> 173: 
> 174:     @Test(dataProvider = "readers", expectedExceptions = IOException.class)

Same comment as above, here and the other test methods, about using assertThrows instead of expectedExceptions at the method level.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793015110
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1793017336