On 6/24/18, 12:11 PM, Alan Bateman wrote:
On 20/06/2018 04:32, Joe Wang wrote:
Thanks Alan. I created 8205058 to capture your suggestions. Please
see below for more details.
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.
JBS: https://bugs.openjdk.java.net/browse/JDK-8205058
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/
I see your point on MalformedInputException and
UnmappableCharacterException so maybe we can submit a new issue to
track the follow-on work.
Submitted: https://bugs.openjdk.java.net/browse/JDK-8205613
The update means a lot of duplication in StringCoding. Did you
consider (or measure) having the private encode/decode methods take a
parameter to indicate the exception handling? Sherman might have
suggestions on this.
I did. I didn't like the code duplication at all. But it would be even
messier if we reuse the code since the previous usage didn't declare
checked exception, but did capture the position (offset) and length
information, which means the new method while unnecessary to capture
these information for Files read/writeString would still need to save
them in case Exception occurs. Because of the complication, I thought
you and Sherman would again prefer a specific methods than adding
parameters (need fields as well).
Furthermore, in the first round of the review, Sherman mentioned that
he's been working on an improvement in this area. But I'll wait till
Sherman could comment on this.
In the tests, shouldn't testMalformedRead and testMalformedWrite be
updated so that expectedExceptions lists the specify exception that is
expected? If I read the update correctly then isn't checking for
MalformedInputException and UnmappableCharacterException anywhere (it
passes if IOException is thrown).
MalformedInputException and UnmappableCharacterException are
implementation details, the tests only verified what the spec required
(IOE).
-Joe
-Alan
