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

[Dmitry Samersoff]

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.

255-258 a) Exceptions could be collapsed

        b) finally is gone - is it expected?
            CONFIG_FILE_CLOSE_FAILED is never happens anymore.

            I would prefer to keep original code


2. ConnectorAddressLink.java
   176 Not sure implicit toString() necessary here.


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


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...