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
