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

[Joe Wang]

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