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

[Mandy Chung]

Dmitry,

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

Why is it not relevant any more?  Hmm.... this code is
a bit confusing. The sun.management.snmp.AdaptorBootstrap class
exists but the snmp runtime classes may not since they are
in the closed source.  The CNFE is from other snmp class but
not sun.management.snmp.AdaptorBootstrap.  Anyway, unless I
miss any recent change, I think the comment is still valid.


> 248        check of existence of configFile is not necessary.
>

Why?  The user can set com.sun.management.config.file to a
non-existence file.  It should catch it.

In any case, this is out of the scope of Kurchi's warning
cleanup work.


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

Good catch.  Let's keep the original code and leave
this for future cleanup.

Mandy


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