8022326: Spliterator for values of j.u.c.ConcurrentSkipListMap does not report ORDERED

Reviewed-by: martin, chegar

jdk/src/share/classes/java/util/TreeMap.java

@@ -2944,16 +2944,11 @@ public class TreeMap<K,V>
 
         @Override
         public Comparator<Map.Entry<K, V>> getComparator() {
-            // Since SORTED is reported and Map.Entry elements are not comparable
-            // then a non-null comparator needs to be returned
+            // Adapt or create a key-based comparator
             if (tree.comparator != null) {
-                // Adapt the existing non-null comparator to compare entries
-                // by key
                 return Map.Entry.comparingByKey(tree.comparator);
             }
             else {
-                // Return a comparator of entries by key, with K assumed to be
-                // of Comparable
                 return (Comparator<Map.Entry<K, V>> & Serializable) (e1, e2) -> {
                     @SuppressWarnings("unchecked")
                     Comparable<? super K> k1 = (Comparable<? super K>) e1.getKey();

jdk/src/share/classes/java/util/concurrent/ConcurrentSkipListMap.java

@@ -34,6 +34,7 @@
  */
 
 package java.util.concurrent;
+import java.io.Serializable;
 import java.util.AbstractCollection;
 import java.util.AbstractMap;
 import java.util.AbstractSet;
@@ -44,11 +45,15 @@ import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NavigableMap;
 import java.util.NavigableSet;
 import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.SortedMap;
+import java.util.SortedSet;
 import java.util.Spliterator;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentNavigableMap;
 import java.util.function.BiFunction;
 import java.util.function.Consumer;
 import java.util.function.BiConsumer;
@@ -108,9 +113,7 @@ import java.util.function.Function;
  * @since 1.6
  */
 public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
-    implements ConcurrentNavigableMap<K,V>,
-               Cloneable,
-               java.io.Serializable {
+    implements ConcurrentNavigableMap<K,V>, Cloneable, Serializable {
     /*
      * This class implements a tree-like two-dimensionally linked skip
      * list in which the index levels are represented in separate
@@ -1412,6 +1415,8 @@ public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
     /**
      * Saves this map to a stream (that is, serializes it).
      *
+     * @param s the stream
+     * @throws java.io.IOException if an I/O error occurs
      * @serialData The key (Object) and value (Object) for each
      * key-value mapping represented by the map, followed by
      * {@code null}. The key-value mappings are emitted in key-order
@@ -1436,6 +1441,10 @@ public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
 
     /**
      * Reconstitutes this map from a stream (that is, deserializes it).
+     * @param s the stream
+     * @throws ClassNotFoundException if the class of a serialized object
+     *         could not be found
+     * @throws java.io.IOException if an I/O error occurs
      */
     @SuppressWarnings("unchecked")
     private void readObject(final java.io.ObjectInputStream s)
@@ -2548,8 +2557,7 @@ public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
      * @serial include
      */
     static final class SubMap<K,V> extends AbstractMap<K,V>
-        implements ConcurrentNavigableMap<K,V>, Cloneable,
-                   java.io.Serializable {
+        implements ConcurrentNavigableMap<K,V>, Cloneable, Serializable {
         private static final long serialVersionUID = -7647078645895051609L;
 
         /** Underlying map */
@@ -3180,6 +3188,7 @@ public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
             public long estimateSize() {
                 return Long.MAX_VALUE;
             }
+
         }
 
         final class SubMapValueIterator extends SubMapIter<V> {
@@ -3434,7 +3443,8 @@ public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
         }
 
         public int characteristics() {
-            return Spliterator.CONCURRENT | Spliterator.NONNULL;
+            return Spliterator.CONCURRENT | Spliterator.ORDERED |
+                Spliterator.NONNULL;
         }
     }
 
@@ -3528,8 +3538,17 @@ public class ConcurrentSkipListMap<K,V> extends AbstractMap<K,V>
         }
 
         public final Comparator<Map.Entry<K,V>> getComparator() {
-            return comparator == null ? null :
-                Map.Entry.comparingByKey(comparator);
+            // Adapt or create a key-based comparator
+            if (comparator != null) {
+                return Map.Entry.comparingByKey(comparator);
+            }
+            else {
+                return (Comparator<Map.Entry<K,V>> & Serializable) (e1, e2) -> {
+                    @SuppressWarnings("unchecked")
+                    Comparable<? super K> k1 = (Comparable<? super K>) e1.getKey();
+                    return k1.compareTo(e2.getKey());
+                };
+            }
         }
     }
 

jdk/test/java/util/Spliterator/SpliteratorCharacteristics.java

@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 8020156 8020009
+ * @bug 8020156 8020009 8022326
  * @run testng SpliteratorCharacteristics
  */
 
@@ -32,79 +32,133 @@ import org.testng.annotations.Test;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.Map;
+import java.util.Set;
+import java.util.SortedMap;
+import java.util.SortedSet;
 import java.util.Spliterator;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.concurrent.ConcurrentSkipListMap;
+import java.util.concurrent.ConcurrentSkipListSet;
 
 import static org.testng.Assert.*;
 
 @Test
 public class SpliteratorCharacteristics {
 
+    // TreeMap
+
     public void testTreeMap() {
-        TreeMap<Integer, String> tm = new TreeMap<>();
-        tm.put(1, "4");
-        tm.put(2, "3");
-        tm.put(3, "2");
-        tm.put(4, "1");
+        assertSortedMapCharacteristics(new TreeMap<>(),
+                                       Spliterator.SIZED | Spliterator.DISTINCT |
+                                       Spliterator.SORTED | Spliterator.ORDERED);
+    }
+
+    public void testTreeMapWithComparator() {
+        assertSortedMapCharacteristics(new TreeMap<>(Comparator.reverseOrder()),
+                                       Spliterator.SIZED | Spliterator.DISTINCT |
+                                       Spliterator.SORTED | Spliterator.ORDERED);
+    }
 
-        assertCharacteristics(tm.keySet(),
-                              Spliterator.SIZED | Spliterator.DISTINCT |
-                              Spliterator.SORTED | Spliterator.ORDERED);
-        assertNullComparator(tm.keySet());
 
-        assertCharacteristics(tm.values(),
-                              Spliterator.SIZED | Spliterator.ORDERED);
-        assertISEComparator(tm.values());
+    // TreeSet
 
-        assertCharacteristics(tm.entrySet(),
-                              Spliterator.SIZED | Spliterator.DISTINCT |
-                              Spliterator.SORTED | Spliterator.ORDERED);
-        assertNotNullComparator(tm.entrySet());
+    public void testTreeSet() {
+        assertSortedSetCharacteristics(new TreeSet<>(),
+                                       Spliterator.SIZED | Spliterator.DISTINCT |
+                                       Spliterator.SORTED | Spliterator.ORDERED);
     }
 
-    public void testTreeMapWithComparator() {
-        TreeMap<Integer, String> tm = new TreeMap<>(Comparator.<Integer>reverseOrder());
-        tm.put(1, "4");
-        tm.put(2, "3");
-        tm.put(3, "2");
-        tm.put(4, "1");
+    public void testTreeSetWithComparator() {
+        assertSortedSetCharacteristics(new TreeSet<>(Comparator.reverseOrder()),
+                                       Spliterator.SIZED | Spliterator.DISTINCT |
+                                       Spliterator.SORTED | Spliterator.ORDERED);
+    }
 
-        assertCharacteristics(tm.keySet(),
-                              Spliterator.SIZED | Spliterator.DISTINCT |
-                              Spliterator.SORTED | Spliterator.ORDERED);
-        assertNotNullComparator(tm.keySet());
 
-        assertCharacteristics(tm.values(),
-                              Spliterator.SIZED | Spliterator.ORDERED);
-        assertISEComparator(tm.values());
+    // ConcurrentSkipListMap
 
-        assertCharacteristics(tm.entrySet(),
-                              Spliterator.SIZED | Spliterator.DISTINCT |
-                              Spliterator.SORTED | Spliterator.ORDERED);
-        assertNotNullComparator(tm.entrySet());
+    public void testConcurrentSkipListMap() {
+        assertSortedMapCharacteristics(new ConcurrentSkipListMap<>(),
+                                       Spliterator.CONCURRENT | Spliterator.NONNULL |
+                                       Spliterator.DISTINCT | Spliterator.SORTED |
+                                       Spliterator.ORDERED);
+    }
+
+    public void testConcurrentSkipListMapWithComparator() {
+        assertSortedMapCharacteristics(new ConcurrentSkipListMap<>(Comparator.<Integer>reverseOrder()),
+                                       Spliterator.CONCURRENT | Spliterator.NONNULL |
+                                       Spliterator.DISTINCT | Spliterator.SORTED |
+                                       Spliterator.ORDERED);
     }
 
-    public void testTreeSet() {
-        TreeSet<Integer> ts = new TreeSet<>();
-        ts.addAll(Arrays.asList(1, 2, 3, 4));
 
-        assertCharacteristics(ts,
-                              Spliterator.SIZED | Spliterator.DISTINCT |
-                              Spliterator.SORTED | Spliterator.ORDERED);
-        assertNullComparator(ts);
+    // ConcurrentSkipListSet
+
+    public void testConcurrentSkipListSet() {
+        assertSortedSetCharacteristics(new ConcurrentSkipListSet<>(),
+                                       Spliterator.CONCURRENT | Spliterator.NONNULL |
+                                       Spliterator.DISTINCT | Spliterator.SORTED |
+                                       Spliterator.ORDERED);
+    }
+
+    public void testConcurrentSkipListSetWithComparator() {
+        assertSortedSetCharacteristics(new ConcurrentSkipListSet<>(Comparator.reverseOrder()),
+                                       Spliterator.CONCURRENT | Spliterator.NONNULL |
+                                       Spliterator.DISTINCT | Spliterator.SORTED |
+                                       Spliterator.ORDERED);
     }
 
-    public void testTreeSetWithComparator() {
-        TreeSet<Integer> ts = new TreeSet<>(Comparator.reverseOrder());
-        ts.addAll(Arrays.asList(1, 2, 3, 4));
 
-        assertCharacteristics(ts,
-                              Spliterator.SIZED | Spliterator.DISTINCT |
-                              Spliterator.SORTED | Spliterator.ORDERED);
-        assertNotNullComparator(ts);
+    //
+
+    void assertSortedMapCharacteristics(SortedMap<Integer, String> m, int keyCharacteristics) {
+        initMap(m);
+
+        boolean hasComparator = m.comparator() != null;
+
+        Set<Integer> keys = m.keySet();
+        assertCharacteristics(keys, keyCharacteristics);
+        if (hasComparator) {
+            assertNotNullComparator(keys);
+        }
+        else {
+            assertNullComparator(keys);
+        }
+
+        assertCharacteristics(m.values(),
+                              keyCharacteristics & ~(Spliterator.DISTINCT | Spliterator.SORTED));
+        assertISEComparator(m.values());
+
+        assertCharacteristics(m.entrySet(), keyCharacteristics);
+        assertNotNullComparator(m.entrySet());
+    }
+
+    void assertSortedSetCharacteristics(SortedSet<Integer> s, int keyCharacteristics) {
+        initSet(s);
+
+        boolean hasComparator = s.comparator() != null;
+
+        assertCharacteristics(s, keyCharacteristics);
+        if (hasComparator) {
+            assertNotNullComparator(s);
+        }
+        else {
+            assertNullComparator(s);
+        }
+    }
+
+    void initMap(Map<Integer, String> m) {
+        m.put(1, "4");
+        m.put(2, "3");
+        m.put(3, "2");
+        m.put(4, "1");
     }
 
+    void initSet(Set<Integer> s) {
+        s.addAll(Arrays.asList(1, 2, 3, 4));
+    }
 
     void assertCharacteristics(Collection<?> c, int expectedCharacteristics) {
         assertCharacteristics(c.spliterator(), expectedCharacteristics);