I'd prefer to go the other way, deleting those trivial methods entirely,
utilizing the rarely used .new syntax.
Index: ConcurrentSkipListMap.java
===================================================================
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/ConcurrentSkipListMap.java,v
retrieving revision 1.147
diff -u -r1.147 ConcurrentSkipListMap.java
--- ConcurrentSkipListMap.java 5 May 2015 16:36:32 -0000 1.147
+++ ConcurrentSkipListMap.java 5 May 2015 17:34:53 -0000
@@ -2311,20 +2311,6 @@
}
}
- // Factory methods for iterators needed by ConcurrentSkipListSet etc
-
- Iterator<K> keyIterator() {
- return new KeyIterator();
- }
-
- Iterator<V> valueIterator() {
- return new ValueIterator();
- }
-
- Iterator<Map.Entry<K,V>> entryIterator() {
- return new EntryIterator();
- }
-
/* ---------------- View Classes -------------- */
/*
@@ -2367,9 +2353,9 @@
}
public Iterator<K> iterator() {
if (m instanceof ConcurrentSkipListMap)
- return ((ConcurrentSkipListMap<K,V>)m).keyIterator();
+ return ((ConcurrentSkipListMap<K,V>)m).new KeyIterator();
else
- return
((ConcurrentSkipListMap.SubMap<K,V>)m).keyIterator();
+ return ((SubMap<K,V>)m).new SubMapKeyIterator();
}
public boolean equals(Object o) {
if (o == this)
@@ -2415,12 +2401,12 @@
public NavigableSet<K> descendingSet() {
return new KeySet<>(m.descendingMap());
}
- @SuppressWarnings("unchecked")
+
public Spliterator<K> spliterator() {
if (m instanceof ConcurrentSkipListMap)
return ((ConcurrentSkipListMap<K,V>)m).keySpliterator();
else
- return (Spliterator<K>)((SubMap<K,V>)m).keyIterator();
+ return ((SubMap<K,V>)m).new SubMapKeyIterator();
}
}
@@ -2431,9 +2417,9 @@
}
public Iterator<V> iterator() {
if (m instanceof ConcurrentSkipListMap)
- return ((ConcurrentSkipListMap<K,V>)m).valueIterator();
+ return ((ConcurrentSkipListMap<K,V>)m).new ValueIterator();
else
- return ((SubMap<K,V>)m).valueIterator();
+ return ((SubMap<K,V>)m).new SubMapValueIterator();
}
public int size() { return m.size(); }
public boolean isEmpty() { return m.isEmpty(); }
@@ -2441,12 +2427,12 @@
public void clear() { m.clear(); }
public Object[] toArray() { return toList(this).toArray(); }
public <T> T[] toArray(T[] a) { return toList(this).toArray(a); }
- @SuppressWarnings("unchecked")
+
public Spliterator<V> spliterator() {
if (m instanceof ConcurrentSkipListMap)
return ((ConcurrentSkipListMap<K,V>)m).valueSpliterator();
else
- return (Spliterator<V>)((SubMap<K,V>)m).valueIterator();
+ return ((SubMap<K,V>)m).new SubMapValueIterator();
}
public boolean removeIf(Predicate<? super V> filter) {
@@ -2454,7 +2440,8 @@
if (m instanceof ConcurrentSkipListMap)
return
((ConcurrentSkipListMap<K,V>)m).removeValueIf(filter);
// else use iterator
- Iterator<Map.Entry<K,V>> it = ((SubMap<K,V>)m).entryIterator();
+ Iterator<Map.Entry<K,V>> it =
+ ((SubMap<K,V>)m).new SubMapEntryIterator();
boolean removed = false;
while (it.hasNext()) {
Map.Entry<K,V> e = it.next();
@@ -2473,9 +2460,9 @@
}
public Iterator<Map.Entry<K,V>> iterator() {
if (m instanceof ConcurrentSkipListMap)
- return ((ConcurrentSkipListMap<K,V>)m).entryIterator();
+ return ((ConcurrentSkipListMap<K,V>)m).new EntryIterator();
else
- return ((SubMap<K,V>)m).entryIterator();
+ return ((SubMap<K,V>)m).new SubMapEntryIterator();
}
public boolean contains(Object o) {
@@ -2517,20 +2504,20 @@
}
public Object[] toArray() { return toList(this).toArray(); }
public <T> T[] toArray(T[] a) { return toList(this).toArray(a); }
- @SuppressWarnings("unchecked")
+
public Spliterator<Map.Entry<K,V>> spliterator() {
if (m instanceof ConcurrentSkipListMap)
return ((ConcurrentSkipListMap<K,V>)m).entrySpliterator();
else
- return (Spliterator<Map.Entry<K,V>>)
- ((SubMap<K,V>)m).entryIterator();
+ return ((SubMap<K,V>)m).new SubMapEntryIterator();
}
public boolean removeIf(Predicate<? super Entry<K,V>> filter) {
if (filter == null) throw new NullPointerException();
if (m instanceof ConcurrentSkipListMap)
return
((ConcurrentSkipListMap<K,V>)m).removeEntryIf(filter);
// else use iterator
- Iterator<Map.Entry<K,V>> it = ((SubMap<K,V>)m).entryIterator();
+ Iterator<Map.Entry<K,V>> it =
+ ((SubMap<K,V>)m).new SubMapEntryIterator();
boolean removed = false;
while (it.hasNext()) {
Map.Entry<K,V> e = it.next();
@@ -3062,18 +3049,6 @@
return descendingMap().navigableKeySet();
}
- Iterator<K> keyIterator() {
- return new SubMapKeyIterator();
- }
-
- Iterator<V> valueIterator() {
- return new SubMapValueIterator();
- }
-
- Iterator<Map.Entry<K,V>> entryIterator() {
- return new SubMapEntryIterator();
- }
-
/**
* Variant of main Iter class to traverse through submaps.
* Also serves as back-up Spliterator for views
Index: ConcurrentSkipListSet.java
===================================================================
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/ConcurrentSkipListSet.java,v
retrieving revision 1.47
diff -u -r1.47 ConcurrentSkipListSet.java
--- ConcurrentSkipListSet.java 15 Jan 2015 18:34:18 -0000 1.47
+++ ConcurrentSkipListSet.java 5 May 2015 17:34:53 -0000
@@ -474,7 +474,7 @@
if (m instanceof ConcurrentSkipListMap)
return ((ConcurrentSkipListMap<E,?>)m).keySpliterator();
else
- return
(Spliterator<E>)((ConcurrentSkipListMap.SubMap<E,?>)m).keyIterator();
+ return ((ConcurrentSkipListMap.SubMap<E,?>)m).new
SubMapKeyIterator();
}
// Support for resetting map in clone
On Tue, May 5, 2015 at 2:12 AM, Paul Sandoz <paul.san...@oracle.com> wrote:
> On May 5, 2015, at 7:54 AM, Martin Buchholz <marti...@google.com> wrote:
> >
> > One query in ConcurrentSkipListMap, we have:
> >
> > 2500 // else use iterator
> > 2501 @SuppressWarnings("unchecked")
> Iterator<Map.Entry<Object,E>> it =
> > 2502 ((SubMap<Object,E>)m).entryIterator();
> >
> > and then
> >
> > 2578 // else use iterator
> > 2579 Iterator<Map.Entry<K1,V1>> it =
> ((SubMap<K1,V1>)m).entryIterator();
> >
> > why does only the former require the "unchecked" warning suppression?
> >
> > Good question.
>
> Yes, for values the key type parameter is "thrown" away and reconstituting
> as Object is not type safe.
>
>
> > I think it's a small but clear improvement to consistently use K,V on
> view subclasses, allowing a few @SuppressWarnings to be removed:
> >
>
> AFAICT all but the above related suppress warning annotations could be
> removed if spliterator returning methods were on SubMap, rather than
> casting the result of the iterator based methods (see end of email for
> patch). I dunno if the 9 compiler got smarter or code was updated and the
> annotation was not removed.
>
> Your patch (plus addition of SubMap spliterator returning methods) seems
> like a good change to bring in bulk-wise, or otherwise earlier on if fixing
> another bug.
>
> Paul.
>
> diff -r b029e73f9cbb
> src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java
> ---
> a/src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java
> Tue May 05 09:43:27 2015 +0200
> +++
> b/src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java
> Tue May 05 11:10:41 2015 +0200
> @@ -2400,12 +2400,12 @@
> Map.Entry<E,?> e = m.pollLastEntry();
> return (e == null) ? null : e.getKey();
> }
> - @SuppressWarnings("unchecked")
> +// @SuppressWarnings("unchecked")
> public Iterator<E> iterator() {
> if (m instanceof ConcurrentSkipListMap)
> - return ((ConcurrentSkipListMap<E,Object>)m).keyIterator();
> + return ((ConcurrentSkipListMap<E,>)m).keyIterator();
> else
> - return
> ((ConcurrentSkipListMap.SubMap<E,Object>)m).keyIterator();
> + return
> ((ConcurrentSkipListMap.SubMap<E,?>)m).keyIterator();
> }
> public boolean equals(Object o) {
> if (o == this)
> @@ -2451,12 +2451,12 @@
> public NavigableSet<E> descendingSet() {
> return new KeySet<E>(m.descendingMap());
> }
> - @SuppressWarnings("unchecked")
> +// @SuppressWarnings("unchecked")
> public Spliterator<E> spliterator() {
> if (m instanceof ConcurrentSkipListMap)
> return ((ConcurrentSkipListMap<E,?>)m).keySpliterator();
> else
> - return (Spliterator<E>)((SubMap<E,?>)m).keyIterator();
> + return ((SubMap<E,?>)m).keySpliterator();
> }
> }
>
> @@ -2465,7 +2465,7 @@
> Values(ConcurrentNavigableMap<?, E> map) {
> m = map;
> }
> - @SuppressWarnings("unchecked")
> +// @SuppressWarnings("unchecked")
> public Iterator<E> iterator() {
> if (m instanceof ConcurrentSkipListMap)
> return ((ConcurrentSkipListMap<?,E>)m).valueIterator();
> @@ -2486,12 +2486,12 @@
> }
> public Object[] toArray() { return toList(this).toArray(); }
> public <T> T[] toArray(T[] a) { return toList(this).toArray(a); }
> - @SuppressWarnings("unchecked")
> +// @SuppressWarnings("unchecked")
> public Spliterator<E> spliterator() {
> if (m instanceof ConcurrentSkipListMap)
> return ((ConcurrentSkipListMap<?,E>)m).valueSpliterator();
> else
> - return (Spliterator<E>)((SubMap<?,E>)m).valueIterator();
> + return ((SubMap<?,E>)m).valueSpliterator();
> }
> public boolean removeIf(Predicate<? super E> filter) {
> if (filter == null) throw new NullPointerException();
> @@ -2516,7 +2516,7 @@
> EntrySet(ConcurrentNavigableMap<K1, V1> map) {
> m = map;
> }
> - @SuppressWarnings("unchecked")
> +// @SuppressWarnings("unchecked")
> public Iterator<Map.Entry<K1,V1>> iterator() {
> if (m instanceof ConcurrentSkipListMap)
> return ((ConcurrentSkipListMap<K1,V1>)m).entryIterator();
> @@ -2563,13 +2563,12 @@
> }
> public Object[] toArray() { return toList(this).toArray(); }
> public <T> T[] toArray(T[] a) { return toList(this).toArray(a); }
> - @SuppressWarnings("unchecked")
> +// @SuppressWarnings("unchecked")
> public Spliterator<Map.Entry<K1,V1>> spliterator() {
> if (m instanceof ConcurrentSkipListMap)
> return
> ((ConcurrentSkipListMap<K1,V1>)m).entrySpliterator();
> else
> - return (Spliterator<Map.Entry<K1,V1>>)
> - ((SubMap<K1,V1>)m).entryIterator();
> + return ((SubMap<K1,V1>)m).entrySpliterator();
> }
> public boolean removeIf(Predicate<? super Entry<K1, V1>> filter) {
> if (filter == null) throw new NullPointerException();
> @@ -3112,14 +3111,26 @@
> return new SubMapKeyIterator();
> }
>
> + Spliterator<K> keySpliterator() {
> + return new SubMapKeyIterator();
> + }
> +
> Iterator<V> valueIterator() {
> return new SubMapValueIterator();
> }
>
> + Spliterator<V> valueSpliterator() {
> + return new SubMapValueIterator();
> + }
> +
> Iterator<Map.Entry<K,V>> entryIterator() {
> return new SubMapEntryIterator();
> }
>
> + Spliterator<Entry<K,V>> entrySpliterator() {
> + return new SubMapEntryIterator();
> + }
> +
> /**
> * Variant of main Iter class to traverse through submaps.
> * Also serves as back-up Spliterator for views
>
>
