Refined type hierarchy of dictionary iterators to be cleaner regardin…

…g AbstractDict and to better match CPython's variant. Simplifies fixing of Stewori/JyNI#13.

src/org/python/core/AbstractDict.java

@@ -6,6 +6,7 @@
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 
 public abstract class AbstractDict extends PyObject {
@@ -65,23 +66,80 @@ static Object tojava(Object val) {
         return val == null ? null : ((PyObject)val).__tojava__(Object.class);
     }
 
-    static class ValuesIter extends PyIterator {
-
-        private final Iterator<PyObject> iterator;
+    /**
+     * Only valid for T=PyObject.
+     * Subclasses can use a different T, but then have to
+     * override __iternext__() accordingly.
+     */
+    static abstract class AbstractDictIter<T> extends PyIterator {
 
-        private final int size;
+        protected final Iterator<T> iterator;
+        protected final int size;
 
-        public ValuesIter(Collection<PyObject> values) {
+        public AbstractDictIter(Collection<T> values) {
             iterator = values.iterator();
             size = values.size();
         }
 
+        /**
+         * For use by subclasses.
+         */
+        protected void check(int size) {
+            if (size != this.size) {
+                throw Py.RuntimeError("dictionary changed size during iteration");
+            }
+        }
+
+        /**
+         * Warning: This default implementation is only
+         * valid for T=PyObject.
+         */
+        @Override
+        public PyObject __iternext__() {
+            if (!iterator.hasNext()) {
+                return null;
+            }
+            return (PyObject) iterator.next();
+        }
+    }
+
+    static class ValuesIter extends AbstractDictIter<PyObject>
+    {
+        public ValuesIter(Collection<PyObject> values) {
+            super(values);
+        }
+    }
+
+    /**
+     * Only valid for T=PyObject.
+     * Subclasses can use a different T, but then have to
+     * override __iternext__() accordingly.
+     */
+    static class KeysIter<T> extends AbstractDictIter<T>
+    {
+        public KeysIter(Collection<T> keys) {
+            super(keys);
+        }
+    }
+
+    /**
+     * Only valid for T=PyObject.
+     * Subclasses can use a different T, but then have to
+     * override __iternext__() accordingly.
+     */
+    static class ItemsIter<T> extends AbstractDictIter<Entry<T, PyObject>> {
+
+        public ItemsIter(Set<Entry<T, PyObject>> items) {
+            super(items);
+        }
+
         @Override
         public PyObject __iternext__() {
             if (!iterator.hasNext()) {
                 return null;
             }
-            return iterator.next();
+            Entry<T, PyObject> entry = iterator.next();
+            return new PyTuple((PyObject) entry.getKey(), entry.getValue());
         }
     }
 }

src/org/python/core/PyDictionary.java

@@ -16,6 +16,8 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.python.core.AbstractDict.ValuesIter;
+import org.python.core.AbstractDict.KeysIter;
+import org.python.core.AbstractDict.ItemsIter;
 import org.python.expose.ExposedClassMethod;
 import org.python.expose.ExposedMethod;
 import org.python.expose.ExposedNew;
@@ -776,7 +778,7 @@ public PyObject iteritems() {
 
     @ExposedMethod(doc = BuiltinDocs.dict_iteritems_doc)
     final PyObject dict_iteritems() {
-        return new ItemsIter(getMap().entrySet());
+        return new ItemsIter<PyObject>(getMap().entrySet());
     }
 
     /**
@@ -788,7 +790,7 @@ public PyObject iterkeys() {
 
     @ExposedMethod(doc = BuiltinDocs.dict_iterkeys_doc)
     final PyObject dict_iterkeys() {
-        return new ValuesIter(getMap().keySet());
+        return new KeysIter<PyObject>(getMap().keySet());
     }
 
     /**
@@ -859,27 +861,6 @@ public PyObject viewitems() {
     public PyObject viewvalues() {
         return super.viewvalues();
     }
-
-    class ItemsIter extends PyIterator {
-
-        private final Iterator<Entry<PyObject, PyObject>> iterator;
-
-        private final int size;
-
-        public ItemsIter(Set<Entry<PyObject, PyObject>> items) {
-            iterator = items.iterator();
-            size = items.size();
-        }
-
-        @Override
-        public PyObject __iternext__() {
-            if (!iterator.hasNext()) {
-                return null;
-            }
-            Entry<PyObject, PyObject> entry = iterator.next();
-            return new PyTuple(entry.getKey(), entry.getValue());
-        }
-    }
 
     public Set<PyObject> pyKeySet() {
         return internalMap.keySet();
@@ -1009,7 +990,7 @@ public PyObject __iter__() {
 
         @ExposedMethod(doc = BuiltinDocs.set___iter___doc)
         final PyObject dict_keys___iter__() {
-            return new ValuesIter(dvDict.pyKeySet());
+            return new KeysIter<PyObject>(dvDict.pyKeySet());
         }
 
         @ExposedMethod(type = MethodType.BINARY, doc = BuiltinDocs.set___ne___doc)

src/org/python/core/PyStringMap.java

@@ -13,6 +13,9 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
+import org.python.core.AbstractDict.ValuesIter;
+import org.python.core.AbstractDict.KeysIter;
+import org.python.core.AbstractDict.ItemsIter;
 import org.python.core.PyMapSet.PySetIter;
 import org.python.expose.ExposedClassMethod;
 import org.python.expose.ExposedMethod;
@@ -663,7 +666,7 @@ public PyObject iteritems() {
 
     @ExposedMethod(doc = BuiltinDocs.dict_iteritems_doc)
     final PyObject stringmap_iteritems() {
-        return new ItemsIter(table.entrySet());
+        return new StringMapItemsIter(table.entrySet());
     }
 
     /**
@@ -677,7 +680,7 @@ public PyObject iterkeys() {
     final PyObject stringmap_iterkeys() {
         /* Python allows one to change the dict while iterating over it, including
            deletion. Java does not. Can we resolve with CHM? */
-        return new KeysIter(table.keySet());
+        return new StringMapKeysIter(table.keySet());
     }
 
     /**
@@ -712,63 +715,50 @@ public boolean isSequenceType() {
         return false;
     }
 
-    private abstract class StringMapIter<T> extends PyIterator {
+    private class StringMapValuesIter extends ValuesIter {
 
-        protected final Iterator<T> iterator;
-
-        private final int size;
-
-        public StringMapIter(Collection<T> c) {
-            iterator = c.iterator();
-            size = c.size();
+        public StringMapValuesIter(Collection<PyObject> c) {
+            super(c);
         }
 
         @Override
         public PyObject __iternext__() {
-            if (table.size() != size) {
-                throw Py.RuntimeError("dictionary changed size during iteration");
-            }
+            check(table.size());
             if (!iterator.hasNext()) {
                 return null;
             }
-            return stringMapNext();
-        }
-
-        protected abstract PyObject stringMapNext();
-    }
-
-    private class StringMapValuesIter extends StringMapIter<PyObject> {
-
-        public StringMapValuesIter(Collection<PyObject> c) {
-            super(c);
-        }
-
-        @Override
-        public PyObject stringMapNext() {
             return iterator.next();
         }
     }
 
-    private class KeysIter extends StringMapIter<Object> {
+    private class StringMapKeysIter extends KeysIter<Object> {
 
-        public KeysIter(Set<Object> s) {
+        public StringMapKeysIter(Set<Object> s) {
             super(s);
         }
 
         @Override
-        protected PyObject stringMapNext() {
+        public PyObject __iternext__() {
+            check(table.size());
+            if (!iterator.hasNext()) {
+                return null;
+            }
             return keyToPy(iterator.next());
         }
     }
 
-    private class ItemsIter extends StringMapIter<Entry<Object, PyObject>> {
+    private class StringMapItemsIter extends ItemsIter<Object> {
 
-        public ItemsIter(Set<Entry<Object, PyObject>> s) {
+        public StringMapItemsIter(Set<Entry<Object, PyObject>> s) {
             super(s);
         }
 
         @Override
-        public PyObject stringMapNext() {
+        public PyObject __iternext__() {
+            check(table.size());
+            if (!iterator.hasNext()) {
+                return null;
+            }
             return itemTuple(iterator.next());
         }
     }