7116445: Miscellaneous warnings in the JDBC/RowSet classes

[Stuart Marks]

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.))

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.

* 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.

* Also in CachedRowSetWriter.java, line 308 can use diamond:

     status = new ArrayList<>(sz);

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)

*  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.

* 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. 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).

Thanks,

s'marks