7116445: Miscellaneous warnings in the JDBC/RowSet classes

[Stuart Marks]

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