On 13/01/2012 9:26 PM, Dmitry Samersoff wrote:
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());
}
Totally disagree - that's not the way to write the exception code. If
you have different actions then you use different catch clauses not
instanceof:
catch (FileNotFoundException fnfe) {
error(CONFIG_FILE_NOT_FOUND, fname);
}
catch (IOException ioe) {
error(CONFIG_FILE_OPEN_FAILED, e.getMessage());
}
David
-----
-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
