RFR 8078645: removeIf(filter) in ConcurrentHashMap removes entries for which filter is false

Martin Buchholz martinrb at google.com
Tue May 5 17:39:22 UTC 2015


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.sandoz at oracle.com> wrote:

> On May 5, 2015, at 7:54 AM, Martin Buchholz <martinrb at 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
>
>



More information about the core-libs-dev mailing list