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

[Stuart Marks]

Hi Daniel,

Comments inline.

On 11/19/12 6:36 AM, Daniel Fuchs wrote:
> On 11/17/12 1:59 AM, Stuart Marks wrote:
>>
>> === 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?

Well, they may be undocumented, but that doesn't mean that nobody depends on 
them. :-/

I think these changes are all binary compatible, since the erasures of the new 
versions are all the same as the raw type.

Source compatibility is a different issue. Actually it doesn't seem too bad. 
The old version returned a raw Enumeration, and the new version returns either 
an Enumeration<?> or Enumeration<SnmpMibSubRequest> or whatever. So callers 
will get a rawtypes warning when they recompile, but only if they enable the 
right -Xlint option.

Given that the compatibility issues seem quite minor, and our assumption that 
it's unlikely that code outside the JDK uses these interfaces, these changes 
seem pretty safe.

Sound reasonable?

>> === 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 ;-)

OK. I just wanted to mention these deletions, since it's a bit unusual to 
delete dead code as part of warnings cleanup. (Do you have a tool that issues 
warnings for dead methods? I don't think javac does.) If you're sure the code 
really is dead -- not just mostly dead :-) -- then it's fine to remove.

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

If you think it makes sense this way, I'm ok with it.

Thanks for acting on my other comments.

s'marks