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

[Stuart Marks]

On 4/5/12 1:12 AM, Rémi Forax wrote:
> On 04/05/2012 09:08 AM, Stuart Marks wrote:
>>
>> 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.

OK, at least I wasn't missing something obvious. :-)

>> 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'm fine with leaving the "extra" rawtypes suppression.

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

Yeah, probably something like "need to cast to raw in order to work around a 
limitation in the type system."

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

I couldn't find it in Doug Lea's JSR166 repository, if that's what you were 
referring to.

Note that a similar code snippet appears in TimSort.java, extra local variable 
and all.

I'm inclined to leave the @SW and extra local variable in place, on the off 
chance that there's some reason for having them that we're unaware of.

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

I assume you disagree with my preference leave unmaskNull() returning V, not 
with joining the lines at L337. :-)

I'm somewhat skeptical of making code changes now based on potential future 
benefits when/if generics become reified. This was discussed before; see

http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000454.html

In that message, John Rose said "If the best practices have to change, then 
we'll have to change that code again.  Or maybe the retrofit strategy will have 
to take account of the existing code idioms.  In any case, we'll cross that 
bridge when we get to it.  (Coping with reification in this case is a decision 
to make tomorrow, not today.)"

That said, if you think that unmaskNull() returning an Object makes sense, even 
after setting aside the reification issue, then I'm ok with this change.


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

I'm more concerned with clarity of source code than extra local variables. 
We've left in extra locals before (ComparableTimSort.java above, and also in 
other files). And when we fix up code in response to warnings from static 
analyzers, we'll have more important things to deal with, I'm sure.

In any case I think your suggested rewrite is fine.

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

Yes, same for these as well.

s'marks