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
