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

[Stuart Marks]

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

  - L2520 declaration of T[] copy



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

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

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



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.

[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



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

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



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.

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


Thanks!!

s'marks



> On 03/30/2012 02:15 AM, Kurchi Hazra wrote:
>> Bug : http://monaco.us.oracle.com/detail.jsf?cr=7157893
>> Webrev: http://cr.openjdk.java.net/~khazra/7157893/webrev.00/
>>
>> Some related discussion that I could find in the core-libs-dev archives:
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008601.html
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008602.html