chore: resolve review comment

Author: fatelei

Lib/test/test_ordered_dict.py

@@ -874,26 +874,6 @@ def side_effect(self):
         self.assertDictEqual(dict1, dict.fromkeys((0, 4.2)))
         self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
-    def test_getitem_re_entrant_clear_during_copy(self):
-        class Evil(self.OrderedDict):
-            def __getitem__(self, key):
-                super().clear()
-                return None
-
-        evil_dict = Evil([(i, i) for i in range(4)])
-        with self.assertRaises(RuntimeError):
-            result = evil_dict.copy()
-
-    def test_getitem_re_entrant_modify_during_copy(self):
-        class Modifier(self.OrderedDict):
-            def __getitem__(self, key):
-                self['new_key'] = 'new_value'
-                return super().__getitem__(key)
-
-        original = Modifier([(1, 'one'), (2, 'two')])
-        with self.assertRaises(RuntimeError):
-            result = original.copy()
-
 
 @unittest.skipUnless(c_coll, 'requires the C version of the collections module')
 class CPythonOrderedDictTests(OrderedDictTests,
@@ -991,6 +971,77 @@ def test_weakref_list_is_not_traversed(self):
 
         gc.collect()
 
+    def test_getitem_re_entrant_clear_during_copy(self):
+        class Evil(self.OrderedDict):
+            def __getitem__(self, key):
+                super().clear()
+                return None
+
+        evil_dict = Evil([(i, i) for i in range(4)])
+        with self.assertRaises(RuntimeError):
+            result = evil_dict.copy()
+
+    def test_getitem_re_entrant_modify_during_copy(self):
+        class Modifier(self.OrderedDict):
+            def __getitem__(self, key):
+                self['new_key'] = 'new_value'
+                return super().__getitem__(key)
+
+        original = Modifier([(1, 'one'), (2, 'two')])
+        with self.assertRaises(RuntimeError):
+            result = original.copy()
+
+    def test_getitem_re_entrant_delete_during_copy(self):
+        class Deleter(self.OrderedDict):
+            call_count = 0
+            def __getitem__(self, key):
+                Deleter.call_count += 1
+                if Deleter.call_count == 1:
+                    del self[3]
+                return super().__getitem__(key)
+
+        original = Deleter([(1, 'one'), (2, 'two'), (3, 'three')])
+        with self.assertRaises(RuntimeError):
+            result = original.copy()
+
+    def test_getitem_re_entrant_add_during_copy(self):
+        class MultiAdder(self.OrderedDict):
+            def __getitem__(self, key):
+                self['new_key1'] = 'new_value1'
+                return super().__getitem__(key)
+
+        original = MultiAdder([(1, 'one'), (2, 'two'), (3, 'three')])
+        with self.assertRaises(RuntimeError):
+            result = original.copy()
+
+    def test_getitem_re_entrant_pop_during_copy(self):
+        class Popper(self.OrderedDict):
+            call_count = 0
+            def __getitem__(self, key):
+                Popper.call_count += 1
+                if Popper.call_count == 1:
+                    self.pop(3, None)
+                return super().__getitem__(key)
+
+        original = Popper([(1, 'one'), (2, 'two'), (3, 'three')])
+        with self.assertRaises(RuntimeError):
+            result = original.copy()
+
+    def test_getitem_re_entrant_mixed_mutation_during_copy(self):
+        class MixedMutator(self.OrderedDict):
+            call_count = 0
+            def __getitem__(self, key):
+                MixedMutator.call_count += 1
+                if MixedMutator.call_count == 1:
+                    del self[3]
+                elif MixedMutator.call_count == 2:
+                    self['new_key'] = 'new_value'
+                return super().__getitem__(key)
+
+        original = MixedMutator([(1, 'one'), (2, 'two'), (3, 'three'), (4, 'four')])
+        with self.assertRaises(RuntimeError):
+            result = original.copy()
+
 
 class PurePythonOrderedDictSubclassTests(PurePythonOrderedDictTests):
 

Objects/odictobject.c

@@ -1268,16 +1268,16 @@ OrderedDict_copy_impl(PyObject *od)
     else {
         PyODictObject *self = _PyODictObject_CAST(od);
         size_t state = self->od_state;
-        _ODictNode *cur = _odict_FIRST(od);
+        _ODictNode *cur;
 
-        while (cur != NULL) {
+        _odict_FOREACH(od, cur) {
             if (self->od_state != state) {
                 PyErr_SetString(PyExc_RuntimeError,
                                 "OrderedDict mutated during iteration");
                 goto fail;
             }
             PyObject *key = Py_NewRef(_odictnode_KEY(cur));
-            PyObject *value = PyObject_GetItem((PyObject *)od, key);
+            PyObject *value = PyObject_GetItem(od, key);
             if (value == NULL) {
                 Py_DECREF(key);
                 goto fail;
@@ -1289,15 +1289,12 @@ OrderedDict_copy_impl(PyObject *od)
                                 "OrderedDict mutated during iteration");
                 goto fail;
             }
-            if (PyObject_SetItem((PyObject *)od_copy, key, value) != 0) {
-                Py_DECREF(key);
-                Py_DECREF(value);
-                goto fail;
-            }
+            int rc = PyObject_SetItem(od_copy, key, value);
             Py_DECREF(key);
             Py_DECREF(value);
-
-            cur = _odictnode_NEXT(cur);
+            if (rc != 0) {
+                goto fail;
+            }
         }
     }
     return od_copy;