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
following command,
make JAVAC_MAX_WARNINGS=true JAVAC_WARNINGS_FATAL= OTHER_JAVACFLAGS="-Xmaxwarns
10000"
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.
s'marks
