7116445: Miscellaneous warnings in the JDBC/RowSet classes

[Lance Andersen - Oracle]

Hi Stuart,

Thanks for the feedback.
On Nov 29, 2011, at 7:31 PM, Stuart Marks wrote:

> On 11/29/11 10:02 AM, Lance Andersen - Oracle wrote:
>> Please review the following changes for JDBC area as part of the warnings clean up day.  I will not be able to participate on Thursday so I wanted to try and get this done ahead of time.
>> 
>> The changes can be found at http://cr.openjdk.java.net/~lancea/7116445/webrev.00/
> 
> Hi Lance, thanks for jumping on the warnings bandwagon!
> 
> I have a few comments and discussion points.
> 
> * In CachedRowSetImpl.java, JdbcRowSetImpl.java and CachedRowSetReader.java, there are places where @SuppressWarnings("deprecation") is added to methods that are fairly long. If during further maintenance of these methods a use of another deprecated element is added, no warning will be issued. It might be nice to try to narrow the scope of the annotation, e.g. for CachedRowSetImpl you could do add this to the acceptChanges() method:
> 
>    @SuppressWarnings("deprecation")
>    final boolean doCommit = this.COMMIT_ON_ACCEPT_CHANGES;
> 
> and then use doCommit where COMMIT_ON_ACCEPT_CHANGES formerly was used. This would allow removal of the SuppressWarnings annotation from the entire method.
> 
> (Changing this to CachedRowSet.COMMIT_ON_ACCEPT_CHANGES would remove additional warnings. Actually since this is a final boolean defined to be true in an interface, it can't be changed, so maybe use of this value can be removed entirely. (But perhaps this is too much JDBC history for you to get into right now.))

I went back and removed this though I was somewhat hesitant due to the history though I am not sure why the original authors of the spec did this anyways (which is why I deprecated it).  Ran the RowSet TCK to validate there were no hiccups (though there should not have been)
> 
> In JdbcRowSetImpl and CachedRowSetReader dealing with the use of deprecated PreparedStatement.setUnicodeStream is trickier. There's no obvious way to add a local declaration on which one can place the annotation. I suppose you could refactor the call into a small method of its own, but this seems like overkill. Up to you as to whether you want to do anything on these.

For now, I would rather leave it as is and will go back and revisit this later after I give it some more thought.
> 
> * 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.
> 
> * Also in CachedRowSetWriter.java, line 308 can use diamond:
> 
>    status = new ArrayList<>(sz);

changed this
> Up to you whether you want to use diamond here since the creation is separate from the declaration.
> 
> Further note that this sets up a potentially similar redundant cast for the result of status.get(), however, the line where it's used does this:
> 
> if (! ((status.get(j)).equals(Integer.valueOf(SyncResolver.NO_ROW_CONFLICT))))
> 
> Although this doesn't generate a warning I think you can simplify this to
> 
> if (status.get(j) != SyncResolver.NO_ROW_CONFLICT)

made the suggested changes
> 
> *  similar redundant cast at line 535, (String)stack.pop(), since stack is now Stack<String>. Oh, again you might want to use diamond where stack is initialized.
made the suggested changes
> 
> * 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

> This usually occurs near a get() call, e.g. in XmlReaderContentHandler, since propMap is now HashMap<String,Integer>, usages can be simplified from
> 
>    tag = ((Integer)propMap.get(name)).intValue();
> 
> to
> 
>    tag = propMap.get(name);
> 
> 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.

The updated webrev is at: http://cr.openjdk.java.net/~lancea/7116445/webrev.01/


Thank you for your review comments

Best
Lance

> 
> Thanks,
> 
> 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