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

Stuart Marks stuart.marks at oracle.com
Thu Apr 5 21:04:58 UTC 2012

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

More information about the core-libs-dev mailing list