7116445: Miscellaneous warnings in the JDBC/RowSet classes

Thu Dec 1 21:21:19 UTC 2011

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.Andersen at oracle.com
>>>> 
>> 
>> 
>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering
>> 1 Network Drive
>> Burlington, MA 01803
>> Lance.Andersen at oracle.com
>> 

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com