Code Review Request: 7117570: Warnings in sun.mangement.* and its subpackages
Mandy Chung
mandy.chung at oracle.com
Fri Jan 13 07:52:54 PST 2012
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
>
More information about the serviceability-dev
mailing list