Address code review

corona10 · Apr 18, 2024 · Apr 17, 2024 · Apr 17, 2024 · Apr 17, 2024 · Apr 17, 2024
commit ae8f217fb1095e03c0b2b71966fbc7e1ee1e2753
diff --git a/Include/internal/pycore_setobject.h b/Include/internal/pycore_setobject.h
@@ -8,15 +8,15 @@ extern "C" {
 #  error "this header requires Py_BUILD_CORE define"
 #endif
 
-// Export for '_pickle' shared extension
+// Export for '_abc' shared extension
 PyAPI_FUNC(int) _PySet_NextEntry(
     PyObject *set,
     Py_ssize_t *pos,
     PyObject **key,
     Py_hash_t *hash);
 
-// Export for 'Python/compile.c'
-PyAPI_FUNC(int) _PyFrozenSet_NextEntry(
+// Export for '_pickle'
+PyAPI_FUNC(int) _PySet_NextEntryRef(
     PyObject *set,
     Py_ssize_t *pos,
     PyObject **key,

diff --git a/Modules/_abc.c b/Modules/_abc.c
@@ -870,22 +870,19 @@ subclasscheck_check_registry(_abc_data *impl, PyObject *subclass,
     Py_ssize_t pos = 0;
     Py_hash_t hash;
 
-    while (_PyFrozenSet_NextEntry(registry, &pos, &key, &hash)) {
+    while (_PySet_NextEntry(registry, &pos, &key, &hash)) {
         PyObject *rkey;
         if (PyWeakref_GetRef(key, &rkey) < 0) {
             // Someone inject non-weakref type in the registry.
             ret = -1;
-            Py_DECREF(key);
             break;
         }
 
         if (rkey == NULL) {
-            Py_DECREF(key);
             continue;
         }
         int r = PyObject_IsSubclass(subclass, rkey);
         Py_DECREF(rkey);
-        Py_DECREF(key);
         if (r < 0) {
             ret = -1;
             break;

diff --git a/Modules/_pickle.c b/Modules/_pickle.c
@@ -3417,7 +3417,7 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj)
 
         int err = 0;
         Py_BEGIN_CRITICAL_SECTION(obj);
-        while (_PySet_NextEntry(obj, &ppos, &item, &hash)) {
+        while (_PySet_NextEntryRef(obj, &ppos, &item, &hash)) {
             err = save(state, self, item, 0);
             Py_CLEAR(item);
             if (err < 0)

diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c
@@ -29,7 +29,7 @@ set_next_entry(PyObject *self, PyObject *args)
     }
     NULLABLE(set);
     Py_BEGIN_CRITICAL_SECTION(set);
-    rc = _PySet_NextEntry(set, &pos, &item, &hash);
+    rc = _PySet_NextEntryRef(set, &pos, &item, &hash);
     Py_END_CRITICAL_SECTION();
     if (rc == 1) {
         return Py_BuildValue("innO", rc, pos, hash, item);

diff --git a/Objects/codeobject.c b/Objects/codeobject.c
@@ -2347,19 +2347,17 @@ _PyCode_ConstantKey(PyObject *op)
             return NULL;
 
         i = 0;
-        while (_PyFrozenSet_NextEntry(op, &pos, &item, &hash)) {
+        while (_PySet_NextEntry(op, &pos, &item, &hash)) {
             PyObject *item_key;
 
             item_key = _PyCode_ConstantKey(item);
             if (item_key == NULL) {
-                Py_DECREF(item);
                 Py_DECREF(tuple);
                 return NULL;
             }
 
             assert(i < len);
             PyTuple_SET_ITEM(tuple, i, item_key);
-            Py_DECREF(item);
             i++;
         }
         set = PyFrozenSet_New(tuple);

@@ -2980,7 +2980,7 @@ dict_set_fromkeys(PyInterpreterState *interp, PyDictObject *mp,
     }
 
     _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(iterable);
-    while (_PySet_NextEntry(iterable, &pos, &key, &hash)) {
+    while (_PySet_NextEntryRef(iterable, &pos, &key, &hash)) {
         if (insertdict(interp, mp, key, hash, Py_NewRef(value))) {
             Py_DECREF(mp);
             mp = NULL;

diff --git a/Objects/listobject.c b/Objects/listobject.c
@@ -1287,7 +1287,7 @@ list_extend_set(PyListObject *self, PySetObject *other)
     Py_hash_t hash;
     PyObject *key;
     PyObject **dest = self->ob_item + m;
-    while (_PySet_NextEntry((PyObject *)other, &setpos, &key, &hash)) {
+    while (_PySet_NextEntryRef((PyObject *)other, &setpos, &key, &hash)) {
         FT_ATOMIC_STORE_PTR_RELEASE(*dest, key);
         dest++;
     }

@@ -2662,37 +2662,36 @@ PySet_Add(PyObject *anyset, PyObject *key)
 }
 
 int
-_PySet_NextEntry_lock_held(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash)
+_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash)
 {
     setentry *entry;
-    int ret = set_next((PySetObject *)set, pos, &entry);
-    if (ret == 0) {
-        return 0;
-    }
-    *key = Py_NewRef(entry->key);
-    *hash = entry->hash;
-    return 1;
-}
 
-int
-_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash)
-{
     if (!PyAnySet_Check(set)) {
         PyErr_BadInternalCall();
         return -1;
     }
-    _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(set);
-    return _PySet_NextEntry_lock_held(set, pos, key, hash);
+    if (set_next((PySetObject *)set, pos, &entry) == 0)
+        return 0;
+    *key = entry->key;
+    *hash = entry->hash;
+    return 1;
 }
 
 int
-_PyFrozenSet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash)
+_PySet_NextEntryRef(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash)
 {
-    if (!PyFrozenSet_CheckExact(set)) {
+    setentry *entry;
+
+    if (!PyAnySet_Check(set)) {
         PyErr_BadInternalCall();
         return -1;
     }
-    return _PySet_NextEntry_lock_held(set, pos, key, hash);
+    _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(set);
+    if (set_next((PySetObject *)set, pos, &entry) == 0)
+        return 0;
+    *key = Py_NewRef(entry->key);
+    *hash = entry->hash;
+    return 1;
 }
 
 PyObject *

diff --git a/Python/compile.c b/Python/compile.c
@@ -920,9 +920,8 @@ merge_consts_recursive(PyObject *const_cache, PyObject *o)
         Py_ssize_t i = 0, pos = 0;
         PyObject *item;
         Py_hash_t hash;
-        while (_PyFrozenSet_NextEntry(o, &pos, &item, &hash)) {
+        while (_PySet_NextEntry(o, &pos, &item, &hash)) {
             PyObject *k = merge_consts_recursive(const_cache, item);
-            Py_DECREF(item);
             if (k == NULL) {
                 Py_DECREF(tuple);
                 Py_DECREF(key);

diff --git a/Python/marshal.c b/Python/marshal.c
@@ -533,7 +533,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
         }
         Py_ssize_t i = 0;
         Py_BEGIN_CRITICAL_SECTION(v);
-        while (_PySet_NextEntry(v, &pos, &value, &hash)) {
+        while (_PySet_NextEntryRef(v, &pos, &value, &hash)) {
             PyObject *dump = _PyMarshal_WriteObjectToString(value,
                                     p->version, p->allow_code);
             if (dump == NULL) {

@@ -2911,12 +2911,11 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp)
             PyObject *item;
             Py_hash_t hash;
             // if stdlib_module_names is not NULL, it is always a frozenset.
-            while (_PyFrozenSet_NextEntry(stdlib_module_names, &i, &item, &hash)) {
+            while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) {
                 if (PyUnicode_Check(item)
                     && PyUnicode_Compare(key, item) == 0)
                 {
                     is_stdlib_ext = 1;
-                    Py_DECREF(item);
                     break;
                 }
             }