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

[Xueming Shen]

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
>>>>