Code Review Request: 7117570: Warnings in sun.mangement.* and its subpackages
Kurchi Hazra
kurchi.subhra.hazra at oracle.com
Fri Jan 13 12:03:23 PST 2012
Updated webrev: http://cr.openjdk.java.net/~khazra/7117570/webrev.07/
Thanks,
Kurchi
On 1/13/2012 7:52 AM, Mandy Chung wrote:
>
>
> On 1/13/2012 3:26 AM, Dmitry Samersoff wrote:
>>> 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.
>>
>
> I see what you mean. Perhaps the comment can be:
> // snmp runtime doesn't exist - initialization fails
>
>>>> 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());
>> }
>>
>>
>
> I agree with David that the above should use different catch clauses
> rather than instanceof within one single catch clause.
>
> Since this is out of the scope of the warning cleanup, I suggest to
> file a bug on this issue and Kurchi will keep the existing code
> unmodified.
>
> Mandy
>
>>
>> -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
>>
--
-Kurchi
More information about the serviceability-dev
mailing list