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

[Kurchi Hazra]

Hi Dmitry,

Thanks for your comments. Please see my doubts/comments inline :

On 1/11/2012 6:34 AM, Dmitry Samersoff wrote:
> Kurchi,
>
> Sorry for stepping into it at later stage, my few cents.
>
> 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.

>          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.
>
> 2. ConnectorAddressLink.java
>     176 Not sure implicit toString() necessary here.
I am not sure too. getValue() returns an Object.


Thanks,
Kurchi


>
> -Dmitry
>
> On 01/11/2012 05:45 AM, Kurchi Hazra wrote:
>>
>> On 1/10/2012 4:58 PM, Stuart Marks wrote:
>>> On 1/10/12 4:35 PM, Kurchi Hazra wrote:
>>>> Updated webrev with all the above changes incorporated:
>>>> http://cr.openjdk.java.net/~khazra/7117570/webrev.04/
>>>>
>>>>> SnmpNamedListTableCache.java
>>>>> L216,221,225: should it be List<?>  rather than List<Object>?
>>>>> Will that help get rid of the unchecked suppressed warning?
>>>> - It does remove the suppressed warning added to this function, but will
>>>> give rise to added suppress warnings in other places. For example:
>>>>
>>>> src/share/classes/sun/management/snmp/util/SnmpListTableCache.java:109
>>>> (since now the argument passed to be has to be List<?>, else the
>>>> compiler
>>>> complains)
>>>>
>>>> Do you still want me to change it to List<?>  and not List<Object>?
>>> I was looking at this too. Arguably the best conversion of a raw type
>>> List to generics is List<?>  but as you noted this causes a ripple
>>> effect, where other things have to be converted to wildcards as well.
>>> It looks like you have to change updateCachedDatas() and
>>> SnmpListTableCache.updateCachedDatas(), but then that's it. That
>>> doesn't look too bad. It could be that I've missed something and that
>>> you'll have to change more and more, in which case I'd reconsider
>>> making the change to use wildcards.
>> This webrev includes the above changes List<Object>  to List<?>
>>
>> http://cr.openjdk.java.net/~khazra/7117570/webrev.05/
>>
>> -Kurchi
>>
>>>>> You mentioned in your previous email that sun.management and its
>>>>> subpackages are warning free with your changeset but I suspect
>>>>> there are still warnings e.g.
>>>>> JvmMemoryImpl.java L160 - this casts the key to MemoryUsage.
>>>> This and I also see some other casts that are somehow not being
>>>> reported even if I turn on -Werror in make/sun/management/snmp. I will
>>>> let this be for the time being and first get this changeset pushed.
>>>> Probably
>>>> in a new CR I will try adding -Werror to make/java/management and
>>>> make/sun/management.
>>> This cast doesn't generate an unchecked warning, because it *is*
>>> checked -- the VM checks the downcast from Object to MemoryUsage and
>>> throws ClassCastException if there's a mismatch. The compiler only
>>> emits unchecked warnings when the cast can't be checked at compile
>>> time *and* it can't be checked at runtime.
>>>
>>> Offhand I don't know whether this code is correct in making this cast.
>>> (The cast is *checked* but it might not be *safe*.) The code does
>>> catch RuntimeException and it does something reasonable, so maybe it's
>>> OK. This effort is about cleaning up warnings, though, and since
>>> there's no warning here I don't think we need to worry about it.
>>>
>>> s'marks
>>>
>>>
>>>>
>>>> Thanks,
>>>> Kurchi
>>>>
>>>>
>>>>> Did you get a chance to check the incremental build and see if
>>>>> there are warnings or not? e.g. cd sun/management; make clean all
>>>>> I suspect the snmp code still has compiler warnings but that's fine
>>>>> since it's very old code that requires more cleanup work for the
>>>>> future.
>>>>>
>>>>> Mandy
>>>>>
>>>>> On 1/9/2012 12:02 PM, Kurchi Hazra wrote:
>>>>>> Hi,
>>>>>>
>>>>>> As an effort to cleanup build warnings, this webrev involves small
>>>>>> changes in
>>>>>> sun.management.* and its subpackages:
>>>>>>
>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7117570
>>>>>> Webrev: http://cr.openjdk.java.net/~khazra/7117570/webrev.03/
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Kurchi
>

-- 
-Kurchi