On 11/17/12 1:59 AM, Stuart Marks wrote:
This is a call for review for
JDK-8003476 Cleanup warnings in com.sun.jmx.snmp code
I have also added @Override annotations wherever they were missing,
but only in the files that had warnings.
<http://cr.openjdk.java.net/~dfuchs/JDK-8003476/webrev.00/>
Hi, overall mostly looks fine. The things that caught my eye were a
number of occurrences of wildcarded generics when it appears that a
specific type could be used.
=== EnumRowStatus.java ===
Possibly join lines shortened by using diamond, if there's room, e.g.
lines 285, 287.
Will do.
=== IPAcl/JJTParserState.java, 32-33 ===
Can this be Vector<Node> instead?
Good catch.
nodes can be Stack<Node>.
marks can be Stack<Integer> as well.
BTW - this code was originally generated by jjtree.
=== IPAcl/Parser.java, line 1171 ===
Should that be Vector<Object> or Vector<?> ? Looks like it really
contains int[]. Maybe Vector<int[]> ?
Vector<?> is not an option - since Vector.addElement() is called.
Vector<int[]> would be the right thing to do. Will do.
=== InetAddressAcl.java ===
The methods getTrapDestinations(), getTrapCommunities(),
getInformDestinations(), getInformCommunities() all list specific types
as elements of returned enumeration in their doc, but all return
Enumeration<?>. Can specific types be used instead?
=== SnmpRequestTree.java ===
The getSubRequests() method returns Enumeration<?>, but it looks like it
could return Enumeration<SnmpMibSubRequest> instead. This would remove
some casts from callers, such as in SnmpMib.java.
These are undocumented sun proprietary public interfaces subject
to change without notice, so backward compile-compatibility is
not enforced - right?
Can we do that in a 'cleanup warnings' bug?
=== SnmpMibAgent.java ===
Did you intend to omit concatVector(Vector, Vector) ?
Yes. That's dead code. The best way to fix warnings in dead code is to
remove the dead code - IMHO ;-)
=== SnmpMibRequest.java and SnmpMibSubRequest.java ===
Can getElements() return Enumeration<SnmpVarBind>? That's what it says
are contained in the enumeration. This will remove some casts in calling
code.
=== SnmpMibTable.java ===
Did you intend to delete getInsertionPoint(SnmpOid) and some associated
javadoc?
Yes. dead code.
Line 2359 typo, "hastable" => "hashtable"
Also, should the values in handbackTable be Vector<Object> or Vector<?>
? It looks like the callers treat them as Vector<?> but it's declared
here as Vector<Object>.
It's not a huge problem since the callers seem to have to do casting
anyway, but the semantics are slightly different. Vector<?> means a
vector of some specific type that's not known here, whereas
Vector<Object> means a vector of objects that are subclasses of Object
-- essentially anything. I'm not sure which is appropriate in this case,
but it doesn't seem quite right to mix them.
They are Vector<Object>. But some time we don't care about what's
in them - so we can manipulate them as Vector<?>. They can't be
declared as Vector<?> however since it would prevent adding
to them...
=== CommunicatorServer.java ===
Looks like fatherThread and loadClass() were removed, presumably they're
dead code? This is a bit beyond what we usually do for warnings cleanup,
although if you're confident these are safe changes I don't object to them.
Yes - dead code. father thread was private, and loadClass
package protected - so we can safely remove them.
=== SnmpRequestHandler.java ===
Can 'mibs' be of type Vector<SnmpMibAgent> ?
Maybe the 'Vector<?> m' constructor argument can also be
Vector<SnmpMibAgent> ?
Yes - it can - good point.
=======
Thanks,
s'marks
