Hi Sherman,
Combining the msg and cause is a great idea! That allowed us to satisfy
the existing code without changes/additional parameter and also the new
method for a CCE. I also moved the iae-catch into StringCoding, that
makes Files clean of such things.
Here's an updated webrev with both suggestions:
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/
Thanks,
Joe
On 6/25/18, 10:28 PM, Xueming Shen wrote:
Hi Joe,
I would suggest always embed a malformed exception into the iae as showed
below, then the extra flag is no longer needed.
private static void throwMalformed(int off, int nb) {
String msg = "malformed input off : " + off + ", length : "
+ nb;
throw new IllegalArgumentException(msg, new
MalformedInputException(nb));
}
It's an implementation details, so we might be able to get rid of the
iae later if we
can figure out the better way to pass the malformed exception for both
ZipCoder/Files
later, without touch the api.
An alternative is to move the iae-catch into StringCoding, with pair
of similar methods
to throw IOE (to add into the JLA), not sure if it's better though. It
makes the Files
impl cleaner at least.
-Sherman
On 6/25/18, 9:50 PM, Joe Wang wrote:
Hi Alan, Sherman,
Here's a version where we, as Sherman suggested, throw an IAE with
CCE as the cause. This approach reduces code duplication in SC,
although it complicates the impl a little bit with the added
parameter and the different behavior between the existing usages of
the methods and the new ones. The existing code paths are kept intact
so there's no compatibility issue for the existing code.
This version also did not remove the try-catch in Files as Alan
suggested earlier.
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/
Thanks,
Joe
On 6/25/18, 3:41 PM, Joe Wang wrote:
Hi Alan,
The test testMalformedRead and testMalformedWrite now expect an
UnmappableCharacterException instead of IOE:
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev01/
Thanks,
Joe
On 6/25/18, 9:48 AM, Joe Wang wrote:
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
