Hi Stuart, On Dec 1, 2011, at 4:10 PM, Stuart Marks wrote:
> Hi Lance, > > Thanks for your patience. I was doing a lot of organizing this morning and > I'm finally getting back to reviewing. Totally understand. I appreciate your time on this as while some of this seems simple, it is a lot of files that are being touched :-) > > Overall the changeset looks fine. I have just a few questions. You might or > might not want to make further changes, but in any case I'm OK if you were to > go ahead and push. > > Items as follows: > > * CachedRowSetImpl.java line 4007: can unboxing be used here to replace > "Integer.valueOf(0)" with simply "0"? This is done below at 4174. Yes it should be able to, i saw this after I hit send (too many passes through this last night. > > * Similar unboxing question at line 401 of CachedRowSetWriter.java Yes I saw this. > > * JdbcRowSetImpl.java and CachedRowSetReader.java: you added "break" in some > switch statements here. This actually changes the behavior of the program and > (probably) fixes a bug, in addition to removing warnings. I guess this is a > bit out of bounds for Warnings Cleanup Day, but I think you have the > prerogative to do this since you're the primary maintainer of this code. I got this as a warning and findbugs was also barking at me about this. The previous code was indeed buggy so I thought I would address it. > > * Row.java has a change to use System.arraycopy(): again a bit much for > warnings day but you have prerogative as the maintainer of this area. Netbeans was gently suggesting this change and I was OK with it so I would prefer to leave it. I will make the couple changes above for the unboxing and generate 1 last webrev and then push it once I get the final green light... Best Lance > > Thanks! > > s'marks > > > > > > On 12/1/11 11:52 AM, Lance Andersen - Oracle wrote: >> 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 >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
