Code Review Request: 7157893: Warnings Cleanup in java.util.* (original) (raw)
Stuart Marks stuart.marks at oracle.com
Thu Apr 5 07:08:24 UTC 2012
- Previous message: Initial preview: JEP-149 Reduced Class instance size
- Next message: Code Review Request: 7157893: Warnings Cleanup in java.util.*
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
L2520 declaration of T[] copy
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). 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.
I was going to suggest adding a comment to explain this but I suspect that would make this code even more confusing.
[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?
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.
[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
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
L442 Oooh, Entry<?,V> -- a half-wildcard! ("Is this a feral type?" -- Joe Darcy) 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
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 to Enumeration. 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. 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. This generates an unchecked warning, so that should be suppressed too.
[4] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000488.html
Thanks!!
s'marks
On 03/30/2012 02:15 AM, Kurchi Hazra wrote:
Bug : http://monaco.us.oracle.com/detail.jsf?cr=7157893 Webrev: http://cr.openjdk.java.net/~khazra/7157893/webrev.00/
Some related discussion that I could find in the core-libs-dev archives: http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008601.html http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008602.html
- Previous message: Initial preview: JEP-149 Reduced Class instance size
- Next message: Code Review Request: 7157893: Warnings Cleanup in java.util.*
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]