Code Review Request: 7117570: Warnings in sun.mangement.* and its subpackages

[David Holmes]

On 13/01/2012 8:50 AM, Dmitry Samersoff wrote:
> On 2012-01-13 02:30, Kurchi Hazra wrote:
>>> 255-258 a) Exceptions could be collapsed
>>
>> This cannot be collapsed since FileNotFoundException is a subclass of
>> IOException.
>
> Does it mean that we can simple remove catch of FileNotFoundException,
> replacing it with a comments that FileNotFound case covered by IOException ?
>
>   263 } catch (FileNotFoundException e) {
>   264             error(CONFIG_FILE_OPEN_FAILED, e.getMessage());
>   265 } catch (IOException e) {
>   266             error(CONFIG_FILE_OPEN_FAILED, e.getMessage());
>
> This code looks really weird for me, could you at least add a comment?

It is redundant to catch FileNotFoundException and IOException and 
perform the same action in each case. This can simply reduce to catching 
IOException.

>>>           b) finally is gone - is it expected?
>>>               CONFIG_FILE_CLOSE_FAILED is never happens anymore.
>>>
>>>               I would prefer to keep original code
>>
>> - We were trying to use try-with-resources (new JDK7 feature), where
>> closing of resources is automatically taken care of. But I will
>> revert back to original code if that is preferred.
>
> We going to an area of changing public symbols. I'm in doubt that
> someone rely on CONFIG_FILE_CLOSE_FAILED message, but we need to check
> it, remove it from all translations etc.
>
> IMHO, It's out of the scope of your amazing work.

I agree. Changing to try-with-resources will change the failure mode. 
I'm not 100% sure how the try-with-resources works in this case but it 
seems to me that an IOException on the close() would result in a call to 
error() with the CONFIG_FILE_OPEN_FAILED message.

David
-----