Review request for java.text.** warning cleanup

[John Rose]

On Dec 1, 2011, at 3:19 PM, Deepak Bhole wrote:

> Hi All,
> 
> Here are all the warning fixes for java.text.**:
> 
> http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.00/
> 
> I have added suppressions where fixes would cause compatibility
> breakage, or where fixes would be too convoluted (e.g the ever so fun
> genericized array creation :)).

Thanks for doing all this work!  I have some review comments.

Bottom line:  In some few cases, I think the the @SuppressWarnings annotations can be refined.  Also, I am worried that there are one or two API signature changes or behavior changes.

Details:

On chunks like this:
-        HashSet keys = new HashSet();
+        HashSet<Attribute> keys = new HashSet<Attribute>();
consider using new HashSet<>().

See also occurrences of new Vector<Attribute>, new HashSet<Map.Entry<Attribute, Object>>, SoftReference<BreakIteratorCache>, etc.

(Netbeans can find opportunities for diamond operators, BTW.)

The new parameters <K extends Attribute, V> are clearly more correct, but they look like an API change on a public method or constructor.  If so, I think you'll have to find a different approach.  (But I'm not an API change and generics expert.)

A trivial refactoring to use a private parameterized method might work nicely, but we're not doing refactorings today.

For createRunAttributeDataVectors, ensureRunBreak, etc. consider pushing the @SuppressWarnings down onto the individual declarations.  It is best to put @SuppressWarnings on the smallest program unit possible, and a local variable declaration is often the right spot.  I see that createRunAttributeDataVectors could be viewed as simple enough to mark as a whole, but ensureRunBreak is probably too complex to mark as a whole.

In the same way, consider pushing the annotation into the body of class AttributeEntry, even though that class is very simple.

For generic array creation, I suggest that you get rid of the "rawtypes" warning on the array element type by adding <?>, before suppressing the "unchecked" warning.  Basically, when you find yourself writing new Foo<Bar>[len], change it to new Foo<?>[len].  Then, if you still get an unchecked warning, add SuppressWarnings.

-            Vector newRunAttributes[] = new Vector[newArraySize];
-            Vector newRunAttributeValues[] = new Vector[newArraySize];
+            @SuppressWarnings("unchecked")  // generic array creation
+            Vector<Attribute> newRunAttributes[] = new Vector<?>[newArraySize];
+            @SuppressWarnings("unchecked")  // generic array creation
+            Vector<Object> newRunAttributeValues[] = new Vector<?>[newArraySize];

It is probably a good idea to put a comment on most @SuppressWarnings annotations.

Similarly:

-    private static final SoftReference[] iterCache = new SoftReference[4];
+
+    @SuppressWarnings({"unchecked","rawtypes"})
+    private static final SoftReference<BreakIteratorCache>[] iterCache = new SoftReference[4];
++    @SuppressWarnings("unchecked")
++    private static final SoftReference<BreakIteratorCache>[] iterCache = new SoftReference<?>[4];

There is an apparent behavior change in CollationElementIterator.setOffset, in this chunk:
-                int c = text.setIndex(newOffset);
+                text.setIndexOnly(newOffset);
+                int c = text.current();

(About 50% through.  More to come...)

Best wishes,
-- John