jmx-dev Review: JDK-8003476 Cleanup warnings in com.sun.jmx.snmp code

[Stuart Marks]

> 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.

=== IPAcl/JJTParserState.java, 32-33 ===

Can this be Vector<Node> instead?

=== IPAcl/Parser.java, line 1171 ===

Should that be Vector<Object> or Vector<?> ? Looks like it really contains 
int[]. Maybe Vector<int[]> ?

=== 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.

=== SnmpMibAgent.java ===

Did you intend to omit concatVector(Vector, Vector) ?

=== 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?

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.

=== 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.

=== SnmpRequestHandler.java ===

Can 'mibs' be of type Vector<SnmpMibAgent> ?

Maybe the  'Vector<?> m' constructor argument can also be Vector<SnmpMibAgent> ?

=======

Thanks,

s'marks