Code Review Request: 7157893: Warnings Cleanup in java.util.*

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<PropertyPermission> to Enumeration<Permission>. 
> 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<T>. 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<Permission>. 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