Code Review Request: 7157893: Warnings Cleanup in java.util.* (original) (raw)

Rémi Forax forax at univ-mlv.fr
Thu Apr 5 08:12:55 UTC 2012

On 04/05/2012 09:08 AM, Stuart Marks wrote:

Hi Kurchi, Remi,

Sorry for the delay in this review. There were a lot of files to go through, and I wanted to dig up some history as well. (I've also been busy....) There are no real major issues, but I wanted to clear a couple things up and possibly file issues for further work. Comments follow on a file-by-file basis.

src/share/classes/java/util/Arrays.java ---------------------------------------------------------------------- In this file there are several places where @SuppressWarnings is placed on a method, which is rather a broad scope. In some cases like mergeSort() it's probably not worth extracting expressions into declarations just to narrow the scope. In other cases there's a declaration handy already, for example those listed below. It would be good to narrow @SW to these declarations. - L2251 declaration of T[] copy

so you want to move the @SupressWarnings on the local variable like this: public static <T,U> T[] copyOf(U[] original, int newLength, Class<? extends T[]> newType) { @SuppressWarnings("unchecked") T[] copy = ((Object)newType == (Object)Object[].class) ? (T[]) new Object[newLength] : (T[]) Array.newInstance(newType.getComponentType(), newLength); ...

I'm Ok this that.

- L2520 declaration of T[] copy

Ok too.

src/share/classes/java/util/Collections.java ---------------------------------------------------------------------- - L1408 this casts to a raw Set in the parameter to super(). There was some discussion about this earlier [1], [2]. David Holmes had suggested casting to Set<? extends Map.Entry<K,V>> but this still generates an unchecked warning. I haven't been able to figure out a cast that doesn't generate an unchecked warning (and I suspect Remi was in the same situation).

You can't in fact. You have to explain to the type system that because the Set is unmodifiable, you can use it in a covariant way, there is no way to do that in Java because you can't put variance annotation at declaration site, but even with that you also need a way to say that all methods that take an E are not implemented and reflect that to the type-system.

So we might as well leave the cast to the raw type. For me, this only generates an unchecked warning, not a rawtypes warning, so maybe we can omit suppression of rawtypes warnings.

Eclipse requires the two declarations. This is, in my opinion, a bug of Eclipse, I will report it but I would prefer to let the two declarations for now.

I was going to suggest adding a comment to explain this but I suspect that would make this code even more confusing.

just perhaps something saying it's a known limitation of the type-system

[1] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000497.html [2] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000501.html

src/share/classes/java/util/ComparableTimSort.java ---------------------------------------------------------------------- - L117 there's a @SuppressWarnings("UnnecessaryLocalVariable") and the local variable newArray really does seem unnecessary. Why not just assign tmp the result of new Object[]? - same at L866 Is there any reason to keep the local variables, for example, if this code came from an upstream repo?

As far as I know, this code was written by Josh and comes from the Doug repository.

src/share/classes/java/util/Currency.java ---------------------------------------------------------------------- - L404 I don't think the @SuppressWarnings here is necessary, as there is @SW inside this method that suppresses a narrower scope. See [3]. I didn't see any other warnings in this method that needed suppression.

You're right, it's a merge issue for me. The annotation should be removed.

[3] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000469.html

src/share/classes/java/util/EnumMap.java ---------------------------------------------------------------------- - I'm not sure why unmaskNull() was changed to return Object instead of V. There are bunch of places elsewhere in the file where it's used and its return value is cast to (V), generating unchecked warnings at those points. It seems to me that it would be preferable to have unmaskNull() return V and have @SW on its declaration than to have @SW sprinkled in a bunch of places elsewhere in the file. This makes unmaskNull() asymmetric with maskNull(), which returns Object, but that's the way the file was originally (so maybe that's OK). - L337 lines can be joined now that wildcards are shorter

I disagree, the casts should be put where they are needed and you don't need them for equals and remove which work with an Object and do not require a V. If one day generics are reified, unlike today, the VM will have to execute some code for these casts, so I prefer to have them only if it's needed even if you have to put more @SW.

src/share/classes/java/util/HashMap.java ---------------------------------------------------------------------- - L393 a @SuppressWarnings is on the declaration of the for-loop index variable? This is somewhat unorthodox. I think it would be better to put the @SW on a new local variable just before the for-loop: @SuppressWarnings("unchecked") Entry<K,V> first = (Entry<K,V>)table[i]; for (Entry<K,V> e = first; e != null; e = e.next) { - L413 similar to above

The unorthodox thing here is the fact that in Java you can declare a variable in a for loop. If you don't want that, I prefer

 @SuppressWarnings("unchecked")
 Entry<K,V> e = (Entry<K,V>)table[i];
 for (; e != null; e = e.next) {

it extends the scope of the variable but it will not be flagged by static bytecode analyzers as an unneeded local variable.

- L442 Oooh, Entry<?,V> -- a half-wildcard! ("Is this a feral type?" -- Joe Darcy)

yes, a kind of semi-domesticated type :)

This is interesting and unusual and inconsistent with the use of Entry<K,V> in the other put*() methods, but it actually makes sense in this case. It's worth a comment that explains that it's this way because e.key is explicitly used as an Object, not as type K.

src/share/classes/java/util/Hashtable.java ---------------------------------------------------------------------- - L440, L479, L703, L1107 more cases of @SW on a for loop variable; change as noted above

so same as above.

src/share/classes/java/util/PropertyPermission.java ---------------------------------------------------------------------- - L599 this is another case of "laundering" a conversion through a raw type (similar to Collections.java above), as we can't directly convert an Enumeration to Enumeration. As Remi noted in [4] this conversion wouldn't be necessary if Collections.enumeration() method were changed to take Collection<?_ _extends T> instead of Collection. But that's an API change and should be handled separately. I'll file a bug on this. Meanwhile leaving the cast to raw is probably reasonable. There should be a comment that mentions the inability to convert directly to Enumeration. This generates an unchecked warning, so that should be suppressed too.

Ok.

[4] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000488.html

Thanks!! s'marks

many thanks too.

Rémi

More information about the core-libs-dev mailing list