On 04/05/2012 09:08 AM, Stuart Marks wrote:
Hi Kurchi, Remi,
Sorry for the delay in this review. There were a lot of files to go
through, and I wanted to dig up some history as well. (I've also been
busy....) There are no real major issues, but I wanted to clear a
couple things up and possibly file issues for further work.
Comments follow on a file-by-file basis.
src/share/classes/java/util/Arrays.java
----------------------------------------------------------------------
In this file there are several places where @SuppressWarnings is
placed on a method, which is rather a broad scope. In some cases like
mergeSort() it's probably not worth extracting expressions into
declarations just to narrow the scope. In other cases there's a
declaration handy already, for example those listed below. It would be
good to narrow @SW to these declarations.
- L2251 declaration of T[] copy
so you want to move the @SupressWarnings on the local variable like this:
public static <T,U> T[] copyOf(U[] original, int newLength, Class<?
extends T[]> newType) {
@SuppressWarnings("unchecked")
T[] copy = ((Object)newType == (Object)Object[].class)
? (T[]) new Object[newLength]
: (T[]) Array.newInstance(newType.getComponentType(),
newLength);
...
I'm Ok this that.
- L2520 declaration of T[] copy
Ok too.
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.
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 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
[1]
http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000497.html
[2]
http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000501.html
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.
src/share/classes/java/util/Currency.java
----------------------------------------------------------------------
- L404 I don't think the @SuppressWarnings here is necessary, as
there is @SW inside this method that suppresses a narrower scope. See
[3]. I didn't see any other warnings in this method that needed
suppression.
You're right, it's a merge issue for me.
The annotation should be removed.
[3]
http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000469.html
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.
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.
- L442 Oooh, Entry<?,V> -- a half-wildcard! ("Is this a feral type?"
-- Joe Darcy)
yes, a kind of semi-domesticated type :)
This is interesting and unusual and inconsistent with the use of
Entry<K,V> in the other put*() methods, but it actually makes sense in
this case. It's worth a comment that explains that it's this way
because e.key is explicitly used as an Object, not as type K.
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.
src/share/classes/java/util/PropertyPermission.java
----------------------------------------------------------------------
- L599 this is another case of "laundering" a conversion through a
raw type (similar to Collections.java above), as we can't directly
convert an Enumeration<PropertyPermission> to Enumeration<Permission>.
As Remi noted in [4] this conversion wouldn't be necessary if
Collections.enumeration() method were changed to take Collection<?
extends T> instead of Collection<T>. But that's an API change and
should be handled separately. I'll file a bug on this.
Meanwhile leaving the cast to raw is probably reasonable. There should
be a comment that mentions the inability to convert directly to
Enumeration<Permission>. This generates an unchecked warning, so that
should be suppressed too.
Ok.
[4]
http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000488.html
Thanks!!
s'marks
many thanks too.
Rémi
