Review request for java.text.** warning cleanup (original) (raw)

John Rose john.r.rose at oracle.com
Thu Dec 1 16:49:11 PST 2011

Previous message: Review request for java.text.** warning cleanup
Next message: Review request for java.text.** warning cleanup
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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, new HashSet<Map.Entry<Attribute, Object>>, SoftReference, 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[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[] iterCache = new SoftReference[4];

++ @SuppressWarnings("unchecked") ++ private static final SoftReference[] 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

Previous message: Review request for java.text.** warning cleanup
Next message: Review request for java.text.** warning cleanup
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the jdk8-dev mailing list