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
