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