Hi Stuart,
On Dec 2, 2011, at 3:45 PM, Stuart Marks wrote:
> 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.
OK, will do that separately
>
> 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. :-)
Probably, but this code has been the gift that keeps on giving :-)
>
> 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)
I can make that change sure and done
>
> 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<?>.
agree done
>
> 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.
done
>
> CachedRowSetWriter.java --
>
> 576 c = (Class)map.get(s.getSQLTypeName());
>
> Redundant cast to (Class).
>
done
> 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.
I thought about making the change as part of this changeset, but thought it
might be too much. Here is the change:
dhcp-adc-twvpn-2-vpnpool-10-154-44-9:rowset lanceandersen$ !hg
hg diff BaseRowSet.java
diff -r 43a630f11af6 src/share/classes/javax/sql/rowset/BaseRowSet.java
--- a/src/share/classes/javax/sql/rowset/BaseRowSet.java Wed Nov 30
13:11:16 2011 -0800
+++ b/src/share/classes/javax/sql/rowset/BaseRowSet.java Fri Dec 02
17:19:53 2011 -0500
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2011, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -619,8 +619,8 @@
checkforRowSetInterface();
if (listeners.isEmpty() == false) {
RowSetEvent event = new RowSetEvent((RowSet)this);
- for (Iterator i = listeners.iterator(); i.hasNext(); ) {
- ((RowSetListener)i.next()).cursorMoved(event);
+ for (RowSetListener rsl : listeners) {
+ rsl.cursorMoved(event);
}
}
}
@@ -644,8 +644,8 @@
checkforRowSetInterface();
if (listeners.isEmpty() == false) {
RowSetEvent event = new RowSetEvent((RowSet)this);
- for (Iterator i = listeners.iterator(); i.hasNext(); ) {
- ((RowSetListener)i.next()).rowChanged(event);
+ for (RowSetListener rsl : listeners) {
+ rsl.rowChanged(event);
}
}
}
@@ -669,8 +669,8 @@
checkforRowSetInterface();
if (listeners.isEmpty() == false) {
RowSetEvent event = new RowSetEvent((RowSet)this);
- for (Iterator i = listeners.iterator(); i.hasNext(); ) {
- ((RowSetListener)i.next()).rowSetChanged(event);
+ for (RowSetListener rsl : listeners) {
+ rsl.rowSetChanged(event);
}
}
}
dhcp-adc-twvpn-2-vpnpool-10-154-44-9:rowset lanceandersen$
>
> (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.
I thought I got this, but looks like i missed it... got it now :-)
>
> *******
>
> 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!! :-)
I made them, thank you...
Ran the RowSet TCK and all still passes for me
for completeness, I did generate a new webrev
http://cr.openjdk.java.net/~lancea/7116445/webrev.04, but will push back if
tonight
Thank you for your help and reviews through all of these modules
Best
Lance
>
> 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
>>
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
