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
