Code Review Request: 7117570: Warnings in sun.mangement.* and its subpackages
Dmitry Samersoff
Dmitry.Samersoff at oracle.com
Fri Jan 13 03:26:46 PST 2012
On 2012-01-13 04:01, Mandy Chung wrote:
> 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.
Comment says
// The SNMP packages are not present: throws an exception.
but with the new code the same exception thrown if package is present
but doesn't have initialize() method or initialize() method has wrong
protection.
i.e. it means that SNMP package is present but invalid.
I'm OK with code changes but would prefer to have comments changed as well.
>> 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.
We are trying to open this file later at ll.260 and would get
FileNotFoundException if the file doesn't exist.
So check on ll.253 is not necessary, moreover it could lead to confusing
results if we are opening file resided on network-mounted media.
It's possible that the file doesn't exist at ll.253 but exists to
ll.260 and vice versa.
So I would prefer to remove this check ever if it's a little bit out of
scope of warning cleanup.
IMHO, exception code
263 } catch (FileNotFoundException e) {
264 error(CONFIG_FILE_OPEN_FAILED, e.getMessage());
265 } catch (IOException e) {
266 error(CONFIG_FILE_OPEN_FAILED, e.getMessage());
should look like:
} catch (IOException e) {
if ( e instanceof FileNotFoundException)
error(CONFIG_FILE_NOT_FOUND, fname);
else
error(CONFIG_FILE_OPEN_FAILED, e.getMessage());
}
-Dmitry
> 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
>>
--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...
More information about the serviceability-dev
mailing list