7116445: Miscellaneous warnings in the JDBC/RowSet classes
stuart.marks at oracle.com
Wed Nov 30 20:58:21 UTC 2011
On 11/30/11 10:43 AM, Lance Andersen - Oracle wrote:
> Hi Stuart,
> Thanks for the feedback.
Great, thanks for the updates. Further comments below.
>> * In CachedRowSetWriter.java, cols is changed from Vector to Vector<Integer>.
>> This is fine but there's now a redundant cast (Integer)cols.get(i) at line
>> 752 that might generate another warning.
> I removed it now. btw, there was no warning generated with cast there which is
> why I did not make the change initially.
I'm not sure what the default compiler flags are for this portion of the build.
I had done a full build of the jdk repo with your patch applied, using the
make JAVAC_MAX_WARNINGS=true JAVAC_WARNINGS_FATAL= OTHER_JAVACFLAGS="-Xmaxwarns
and I did see that some redundant cast warnings were introduced.
>> * WebRowSetXmlWriter.java, XmlReaderContentHandler.java, SQLInputImpl.java,
>> and SyncFactory.java similar removals of redundant casts can be performed for
>> types that have been converted to generics.
> I did not see the changes you were suggesting in SyncFactory but maybe I have
> not had enough coffee :-), was there something specific you saw as I did not
> see an obvious change to get rid of the couple of casts to the type that was
> converted to use generics
Oh yes, the SyncFactory one is different. Usually generifying a type will
introduce redundant casts near calls to things like get(). The
"implementations" field was generified, and I saw this:
ProviderImpl impl = (ProviderImpl) implementations.get(providerID);
which looked like a redundant cast. But the declared type is now
Hashtable<String,SyncProvider> which means the cast isn't redundant, but it
does raise a different set of questions. The "impl" local is only tested for
being null so maybe its type should be SyncProvider instead. Oddly it doesn't
appear to be used if it's non-null. Hm, this code has other issues as well...
(A null return from Class.forName is handled, which AFAIK cannot occur;
c.newInstance() is cast to SyncProvider but ClassCastException isn't handled.)
Despite this I don't actually see any compiler warnings from this code. Since
this is warnings cleanup day maybe we're done. :-) Perhaps further cleanups in
this area can be deferred.
>> You might also consider using diamond where new generic instances are
>> created, though it's a matter of style whether to use diamond in an
>> assignment statement (as opposed to in a declaration's initializer).
> Do we have a preference going forward? I used diamond as you suggested.
My recommendation is to use diamond in field and local initializers and in
simple assignment statements. I avoid diamond in parameters to method calls and
within ternary (?:) expressions.
Some people don't want to use diamond in assignment statements, since the
redundancy isn't as obvious, and they think that having the type arguments in
the constructor is helpful there. But I don't see it.
> The updated webrev is at: http://cr.openjdk.java.net/~lancea/7116445/webrev.01/
Oh... one more thing. Sorry I missed this in my previous review.
In SQLOutputImpl the constructor parameters got changed from Vector<?> to
Vector<Object> and from Map<String,?> to Map<String,Class<?>>. This looks like
a public constructor to a public API class. (At least, I see this in the
javadoc.) This change might actually be correct (though perhaps not strictly
compatible) but I don't think we want to be making such changes as part of
warnings cleanup, even if it does end up removing warnings.
More information about the core-libs-dev