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

[Daniel Fuchs]

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