On 2012-01-13 02:30, Kurchi Hazra wrote:
>> 1. Agent.java
>>
>> 220 comments is not relevant any more.
>>
>> 248 check of existence of configFile is not necessary.
>
> Thanks, I removed these.
>
>> 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?
>
>> 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.
>>
>> 2. ConnectorAddressLink.java
>> 176 Not sure implicit toString() necessary here.
> I am not sure too. getValue() returns an Object.
OK.
Thank you!
-Dmitry
--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...
