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,

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