7116445: Miscellaneous warnings in the JDBC/RowSet classes
Lance Andersen - Oracle
Lance.Andersen at oracle.com
Fri Dec 2 23:02:13 UTC 2011
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.Andersen at 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.Andersen at 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.Andersen at oracle.com
>>
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com
More information about the core-libs-dev
mailing list