RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file

[Joe Wang]

Thanks Alan.  I created 8205058 to capture your suggestions. Please see 
below for more details.

On 6/14/18, 4:30 AM, Alan Bateman wrote:
> 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.

Changed the internal APIs to throw CCE instead. In the same way as the 
previous changeset for 8201276, these methods are made specific for the 
use cases (though they are now for Files.read/writeString only) so that 
they are not mixed up with existing ones that may inadvertently affect 
other usages.

One thing to note is that MalformedInputException or 
UnmappableCharacterException will lose one piece of information in 
comparison to the existing IAE, that is where it happens (offset). 
Should there be an improvement in the future, we could consider add it 
back to this part of code.
>
> 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).

Moved JLA and others to the top. Fixed also the indentations (mostly 
method declarations) and a few long lines.
>
> 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.

Fixed too.

JBS: https://bugs.openjdk.java.net/browse/JDK-8205058
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/


Regards,
Joe

>
> -Alan
>