RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file

[Alan Bateman]

On 12/06/2018 17:52, Joe Wang wrote:
> Hi all,
>
> It's been a while since the 1st round of reviews. Thank you all for 
> the valuable comments and suggestions!  Note that Roger's last 
> response went to nio-dev, but not core-libs-dev, I've therefore 
> attached it below.
>
> CSR [2]: the CSR is now approved. Note the write method has been 
> changed to writeString.
>
> Impl [3]: for performance reason and the different requirement from 
> byte-string conversion for malformed/unmappable character handing, the 
> implementation uses a specific method separate from the existing ones. 
> Both Sherman and Alan preferred specific method than adding parameters 
> to the APIs that might be error prone. Thanks Sherman for the code!
>
> JMH benchmark tests showed the new read method performs better than an 
> operation that reads the bytes and convert to string. The gains start 
> to be significant even at quite reasonable sizes (< 10K).
>
>
> [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055
> [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/
>
> Please review.
The javadoc looks good.

One part of the implementation that could be cleaner is the exception 
thrown for the malformed or unmappable cases. There are sub-classes of 
CharacterCodingException defined for this that would be clearer than an 
IOException with an IllegalArgumentException as cause.

A minor nit in Files is that you can import 
jdk.internal.misc.JavaLangAccess rather than repeating the fully 
qualified class name. You can also move the definition of JLA up to the 
top. There's also a few inconsistencies with the existing code that 
would be goof to fix too (indenting and line length issues mostly).

The test looks reasonable. In getData() then then "shouldn't happen" 
case should throw an exception as a NPE here might be tricky to diagnose 
there. Another nit is the sb field - can that be removed.

-Alan