Here is another round of diffs as I found more warnings after I changed lint levels
http://cr.openjdk.java.net/~lancea/7116445/webrev.02/ For SQLOutputImpl.java, as I found no easy way to avoid the errors without changing the constructor generic types, I went the route of suppressing the warnings in each of the methods. This class is not used much if at all so I am not sure it is worth any more cycles currently. Thanks for your time on the reviews... Best Lance On Nov 30, 2011, at 3:58 PM, Stuart Marks wrote: > > > 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 > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
