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
