Thank you for the review Chris. Once I hear back from Stuart, I will push these changes back.
Best Lance On Dec 1, 2011, at 11:17 AM, Chris Hegarty wrote: > Skimming over this, I think it looks fine. > > -Chris. > > On 01/12/2011 01:46, Lance Andersen - Oracle wrote: >> 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 >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
