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
