Review request for java.text.** warning cleanup

[Rémi Forax]

Hi Deepak,

On 12/02/2011 07:47 PM, Deepak Bhole wrote:
> * 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.

You can use them everywhere but in langtools because
the compiler need to bootstrap itself.

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

It seems you still use clone,
you can't replace a call to clone with a call to a copy constructor because
if Stack is subclassed, clone will work but not the copy constructor.

>
> 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<?>[]

Yes, you need to add a cast,

Vector<Attribute>[] newRunAttributes = (Vector<Attribute>[])new Vector<?>[ARRAY_SIZE_INCREMENT];

but if Java is reified in 1.9 as state the current plan, we will be in 
trouble.

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

cheers,
Rémi