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

[Rémi Forax]

On 04/05/2012 11:04 PM, Stuart Marks wrote:

[...]

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

your English is better than mine :)

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

After a dig up session, Martin Buchholz proposes the patch from sources 
written by Josh
from the Android source repository.
see 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-June/001937.html

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

I agree.

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

yes.

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

I disagree with John. The main issue with generics nowadays is that
most of the people doesn't care about a cast to a type variable because
everybody knows about erasure. So codes are written with an 
implementation glitch in mind.
Frankly, I don't know if reification will appear (yes it's a kind of 
magical) or not
but I think it's a sloppy path to not consider all casts as equals.

If unmaskNull return a V, the code of equals will upcast the value from 
Object to V
to just after downcast it from V to Object,
I think it's better that unmask to return Object and upcast it to V when 
it's necessary.

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

see above.

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

We are currently fixing warnings from a static analyzer :)

For me, letting an extra local variable is not a big deal because perf 
tests was already done on that code
moreover if my memory is good, this extra local variable is not in the 
middle of the loops of the sort.
Here, you want to add a new local variable and I know by experience that 
if you have no luck
that can be enough for Hotspot to don't inline a method that was 
previously inlined.
Even the proposed change as an impact, too long to explain here, but it 
can make the size of the method
a little bigger.
Anyway, you're right that clarity is more important than any of these 
details.

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

cheers,
Rémi