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.ander...@oracle.com
