Hi Lance,
I'm OK with postponing the @Deprecated work, and doing a separate pass for
@Deprecated, including the com.sun.* stuff at that time.
There's enough stuff in this changeset already. I think we're better off
getting it in now than putting more stuff in and getting it reviewed again. I
think you've done this to yourself on this review twice already. :-)
Comments on webrev.03 follow.
CachedRowSetImpl.java --
451 @SuppressWarnings("rawtypes")
452 public CachedRowSetImpl(Hashtable env) throws SQLException {
The warning suppression covers the entire constructor. You could do this...
public CachedRowSetImpl(@SuppressWarnings("rawtypes") Hashtable env)
Ugh. As you said earlier, probably not worth spending more cycles on this.
5710 Class<?> c = (Class)map.get(s.getSQLTypeName());
The cast to (Class) is probably unnecessary since the values in the map are
already Class<?>.
JoinRowSetImpl.java --
932 (vecRowSetsInJOIN.get(i)).getMatchColumnNames()[0]);
4179 Integer i = (vecJoinType.get(vecJoinType.size()-1));
Can probably remove extra set of parens that were necessary when the cast was
there.
CachedRowSetWriter.java --
576 c = (Class)map.get(s.getSQLTypeName());
Redundant cast to (Class).
BaseRowSet.java --
622 for (Iterator<?> i = listeners.iterator(); i.hasNext(); ) {
623 ((RowSetListener)i.next()).cursorMoved(event);
624 }
Hm, listeners is Vector<RowSetListener> so some casts could be removed,
622 for (Iterator<RowSetListener> i = listeners.iterator();
i.hasNext(); ) {
623 (i.next()).cursorMoved(event);
624 }
or even changed to
622 for (RowSetListener rsl : listeners) {
623 rsl.cursorMoved(event);
624 }
There are several other cases in this file where you used Iterator<?>. You
might check them to see if similar simplifications could be done there as well.
(There is also Iterator<?> used in CachedRowSetImpl.java but this is used to
iterate over rvh, a Vector<Object>, but then the elements are cast to Row. This
begs the question of why rvh isn't Vector<Row> but this is beyond the scope of
warnings cleanup.)
SerialArray.java --
290 default:
291 ;
I'm surprised you didn't remove these lines as you had done above.
*******
With the exception of changing Iterator<?> to use a for-each loop in
BaseRowSet.java, all of these comments are just little tweaks, so I don't think
we need another webrev for them. It's up to you whether to proceed with the
BaseRowSet changes I recommend. Regardless, I don't want to see another webrev
of this code. Check it in!! :-)
s'marks
On 12/2/11 8:06 AM, Lance Andersen - Oracle wrote:
On Dec 2, 2011, at 10:54 AM, David Schlosnagle wrote:
On Fri, Dec 2, 2011 at 8:19 AM, Lance Andersen -
Oracle<lance.ander...@oracle.com> wrote:
Adding @Deprecated changes the signatures so I need to coordinate any changes
as it will result in TCK signature failures. This is something I will most
likely do as part of the JDBC 4.2 work after giving the JDBC EG a chance for
input.
Lance,
I figured it was worth a shot to try and piggy back on your changes to cleanup
the rest of java.sql.* :)
Appreciate the suggestion.
I assume it is the @Deprecated annotation's retention runtime value that
introduces TCK signature compatibility issues, is that correct?
Yes, here is the reason why:
@Documented
@Retention(value=RUNTIME)
@Target(value={CONSTRUCTOR,FIELD,LOCAL_VARIABLE,METHOD,PACKAGE,PARAMETER,TYPE})
public @interface Deprecated
why Here is @Documented
@Documented
@Retention(value=RUNTIME)
@Target(value=ANNOTATION_TYPE)
public @interface Documented
Indicates that annotations with a type are to be documented by javadoc and
similar tools by default. This type should be used to annotate the declarations
of types whose annotations affect the use of annotated elements by their
clients. If a type declaration is annotated with Documented, its annotations
become part of the public API of the annotated elements.
Since:
1.5
As you can see this results in an API change which would result in the
signatures changing. This type of change would be flagged by the signature
tests so we would not want to do this in say JDK 7. It would be OK for JDK 8
but I would not want to do it as part of the cleanup changes.
It is interesting that the behavior is different between @Deprecated vs
@deprecated(javadoc mark up) and I have had previous discussions on this as one
vendor made this innocent change in a standalone technology and then could not
pass the signature tests for a given TCK.
Again, my desire is to do this soon, and might for the com.* classes assuming
Alan/Stuart see no issue as part of this push
Many thanks for your time
Best
Lance
If so, that is an interesting catch-22 when the idea behind @Deprecated is to
maintain a backward compatible API for some period to be determined to the API
provider. It seems like this might be worthwhile mentioning in the deprecation
guide [1], and maybe even additions to Joe Darcy's excellent treatises on
compatibility [2] [3]. I completely understand wanting to wait for the
appropriate approvals to move forward with the remaining cleanup.
On Fri, Dec 2, 2011 at 9:14 AM, Lance Andersen -
Oracle<lance.ander...@oracle.com> wrote:
Here is the diff for DriverManager, I won't be pushing another webrev unless
the word is to go ahead and add @Deprecated to the com/* classes of the RowSet
RI or there is another change requested that is more detailed:
That looks good to me, unfortunately I don't think I can be used as a reviewer.
Thanks,
Dave
[1]:
http://docs.oracle.com/javase/6/docs/technotes/guides/javadoc/deprecation/deprecation.html
[2]: http://blogs.oracle.com/darcy/entry/kinds_of_compatibility
[3]: http://blogs.oracle.com/darcy/entry/release_types_compatibility_regions
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
