Review request for java.text.** warning cleanup

[Deepak Bhole]

* John Rose <john.r.rose at oracle.com> [2011-12-01 19:49]:
> 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.
> 

Ah, I wasn't sure if I should have used 7-specific features so I stayed
away from them (when bootstrapping when ecj, we found diamond to be
problematic as older ecj versions cannot understand it). I will change
these then.

> (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.)
> 

Do you mean the changes in the AttributedString constructor? I just
added names for the bounded and unbound wildcards so that they could be
used in the body (without it, the compiler was throwing errors).

I am not expert on these either -- I didn't know adding names could
change signature in an incompatible way.

I just wrote a small test case to try this out and everything seems to
work fine. Adding named parameters does not appear to break
compatibility in my test case.

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

Yeah, I wanted to stay away from anything that added/removed any code :)

> 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.
> 

Doh! Good point.

For DictionaryBasedBreakIterator.divideUpDictionaryRange, I cannot add
it above the 
"bestBreakPositions = (Stack<Integer>)(currentBreakPositions.clone());" line
without refactoring. I done done a very minor refactor (use copy
constructor instead of clone) -- please let me know if it is
satisfactory.

As for others, I've narrowed it in the new webrev. Narrowing also
exposed a few more places I had missed in ensureRunBreak. Fixed now.

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

That one is for the declaration itself -- the warning is about the Map
supertype not being parameterized.

> 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];
> 

I tried that yesterday, but the compiler threw errors:

../../../src/share/classes/java/text/AttributedString.java:420: error: incompatible types
        Vector<Attribute> newRunAttributes[] = new Vector<?>[ARRAY_SIZE_INCREMENT];
                                               ^
  required: Vector<Attribute>[]
  found:    Vector<?>[]

> 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];
> 

Similar error as above.

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

setIndex is deprecated, so I looked into what the method did and did it
at the call site. Sorry, I should have specifically mentioned this
re-factor in the original email :( 

I am fine with just suppressing deprecation warning there. I have done
so in the new copy by adding the annotation to the method.

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

(from the second email):

> Here's the rest.
> 
> I think the method divideUpDictionaryRange is too complex (181 LOC) to be covered by one @SuppressWarnings.  Even adding 30 annotations in the body would be better, since then the other 151 LOC would remain protected.
> 
> I do like the informative comment on @SuppressWarnings:
> +    @SuppressWarnings("unchecked") // suppress unchecked warning for cloning stack
> 

Fixed with a minor re-factor as mentioned above.

> Extra parens (only needed when there was a cast) can go away:
> +            cachedBreakPositions[i + 1] = (currentBreakPositions.elementAt(i)).intValue();
> ++            cachedBreakPositions[i + 1] = currentBreakPositions.elementAt(i).intValue();
> 

Oops. Fixed in all locations (there were 3 others).

> This one has to be on the method level, of course.  Good comment; suggest s/fallthrough/fallthrough in switch/.
> +    @SuppressWarnings("fallthrough") // fallthrough is expected, suppress it
> 

Done!

> Diamond operator, again:
> +        ArrayList<AttributedCharacterIterator> iterators = new ArrayList<AttributedCharacterIterator>();
> ++        ArrayList<AttributedCharacterIterator> iterators = new ArrayList<>();
> 
> and:
> +    private static final Hashtable<Locale, String[]> cachedLocaleData = new Hashtable<Locale, String[]>(3);
> ++    private static final Hashtable<Locale, String[]> cachedLocaleData = new Hashtable<>(3);
> +        private static final Map<String, Field> instanceMap = new HashMap<String, Field>(11);
> ++        private static final Map<String, Field> instanceMap = new HashMap<>(11);
> +            entryTable = new Vector<EntryPair>(INITIALTABLESIZE);
> ++            entryTable = new Vector<>(INITIALTABLESIZE);
> 

Fixed all updates in the patch to use diamond.

> 
> This one has to be at class level; good comment:
> + at SuppressWarnings("serial") // Adding serial ID will break compatibility. Suppress it. 
> 

I saw Stuart's comments on this. Didn't know that there was a way to find
existing serialid (I thought Eclipse just generated a random one)! I have added
the class default id.

> There is a peculiar space in the "int []" type name; consider removing it:
> +                          Vector<int []> eTbl,
> ++                          Vector<int[]> eTbl,
> 
> +    private Vector<int []> expandTable = null;
> ++    private Vector<int[]> expandTable = null;
> +            expandTable = new Vector<int []>(INITIALTABLESIZE);
> ++            expandTable = new Vector<int[]>(INITIALTABLESIZE);
> +    private Vector<int []>   expandTable = null;

Ah, I copied what was there before in RBTableBuilder:
int[] valueList = (int [])expandTable.elementAt(i);

Fixed everywhere.


New webrev:
http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.01/

For convenience, I've also uploaded a diff between previous webrev and
new one to make it clear as to what changed:
http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.01/rev-00-01.patch

Thanks for the quick and detailed review!

Cheers,
Deepak