MAINT: Address review comments

First, use `goto fail` unconditionally, and second try to simplify the tests by relying only on ctypes private (MetaClass using) API.
python · Oct 20, 2021 · 881ba13 · 881ba13
1 parent 2012aaa
commit 881ba13
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 68 deletions.
diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
@@ -24,6 +24,9 @@
 #include <float.h>
 #include <signal.h>
 
+#include <ffi.h>  /* required by ctypes.h */
+#include "_ctypes/ctypes.h"  /* To test metaclass inheritance */
+
 #ifdef MS_WINDOWS
 #  include <winsock2.h>         /* struct timeval */
 #endif
@@ -1173,48 +1176,13 @@ test_get_type_name(PyObject *self, PyObject *Py_UNUSED(ignored))
 }
 
 
-/*
- * Small helper to import abc.ABC and ctypes.Array for testing.  Both
- * are (incompatible) MetaClass instances.  If Array is NULL it is not filled.
- */
-static int
-import_abc_and_array(PyObject **ABC, PyObject **Array)
-{
-    PyObject *abc_mod = PyImport_ImportModule("abc");
-    if (abc_mod == NULL) {
-        return -1;
-    }
-    *ABC = PyObject_GetAttrString(abc_mod, "ABC");
-    Py_DECREF(abc_mod);
-    if (*ABC == NULL) {
-        return -1;
-    }
-    if (Array == NULL) {
-        return 0;
-    }
-
-    PyObject *ctypes_mod = PyImport_ImportModule("ctypes");
-    if (ctypes_mod == NULL) {
-        Py_CLEAR(*ABC);
-        return -1;
-    }
-    *Array = PyObject_GetAttrString(ctypes_mod, "Array");
-    Py_DECREF(ctypes_mod);
-    if (*Array == NULL) {
-        Py_CLEAR(*ABC);
-        return -1;
-    }
-    return 0;
-}
-
-
 static PyType_Slot MinimalType_slots[] = {
     {0, 0},
 };
 
 static PyType_Spec MinimalType_spec = {
     "_testcapi.MinimalSpecType",
-    0,
+    sizeof(CDataObject),
     0,
     Py_TPFLAGS_DEFAULT,
     MinimalType_slots
@@ -1224,31 +1192,22 @@ static PyType_Spec MinimalType_spec = {
 static PyObject *
 test_from_spec_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(ignored))
 {
-    /* Get two (incompatible) MetaTypes */
-    PyObject *ABC;
-    if (import_abc_and_array(&ABC, NULL) < 0) {
-        return NULL;
-    }
-
-    PyObject *bases = PyTuple_Pack(1, ABC);
+    PyObject *bases = PyTuple_Pack(1, PyCArray_Type);
     if (bases == NULL) {
-        Py_DECREF(ABC);
         return NULL;
     }
+
     PyObject *new = PyType_FromSpecWithBases(&MinimalType_spec, bases);
     Py_DECREF(bases);
     if (new == NULL) {
-        Py_DECREF(ABC);
         return NULL;
     }
-    if (Py_TYPE(new) != Py_TYPE(ABC)) {
+    if (Py_TYPE(new) != Py_TYPE(&PyCArray_Type)) {
         PyErr_SetString(PyExc_AssertionError,
                 "MetaType appears not correctly inherited from ABC!");
-        Py_DECREF(ABC);
         Py_DECREF(new);
         return NULL;
     }
-    Py_DECREF(ABC);
     Py_DECREF(new);
     Py_RETURN_NONE;
 }
@@ -1257,19 +1216,11 @@ test_from_spec_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(ignored)
 static PyObject *
 test_from_spec_invalid_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(ignored))
 {
-    /* Get two (incompatible) MetaTypes */
-    PyObject *ABC, *Array;
-
-    if (import_abc_and_array(&ABC, &Array) < 0) {
-        return NULL;
-    }
-
-    PyObject *bases = PyTuple_Pack(2, ABC, Array);
-    Py_DECREF(ABC);
-    Py_DECREF(Array);
+    PyObject *bases = PyTuple_Pack(2, PyCArray_Type, PyCFuncPtr_Type);
     if (bases == NULL) {
         return NULL;
     }
+
     /*
      * The following should raise a TypeError due to a MetaClass conflict.
      */
@@ -1286,7 +1237,7 @@ test_from_spec_invalid_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(
 
         PyErr_Fetch(&type, &value, &traceback);
         Py_DECREF(type);
-        Py_XDECREF(traceback);
+        Py_DECREF(traceback);
 
         meta_error_string = PyUnicode_FromString("metaclass conflict:");
         if (meta_error_string == NULL) {

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
@@ -3360,7 +3360,7 @@ PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases)
 PyObject *
 PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
 {
-    PyHeapTypeObject *res;
+    PyHeapTypeObject *res = NULL;
     PyObject *modname;
     PyTypeObject *type, *base;
     int r;
@@ -3401,7 +3401,7 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
     if (spec->name == NULL) {
         PyErr_SetString(PyExc_SystemError,
                         "Type spec does not define the name field.");
-        return NULL;
+        goto fail;
     }
 
     /* Adjust for empty tuple bases */
@@ -3418,11 +3418,11 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
         if (!bases) {
             bases = PyTuple_Pack(1, base);
             if (!bases)
-                return NULL;
+                goto fail;
         }
         else if (!PyTuple_Check(bases)) {
             PyErr_SetString(PyExc_SystemError, "Py_tp_bases is not a tuple");
-            return NULL;
+            goto fail;
         }
         else {
             Py_INCREF(bases);
@@ -3431,22 +3431,21 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
     else if (!PyTuple_Check(bases)) {
         bases = PyTuple_Pack(1, bases);
         if (!bases)
-            return NULL;
+            goto fail;
     }
     else {
         Py_INCREF(bases);
     }
 
-    /* NOTE: Missing API to replace `&PyType_Type` below, see bpo-15870 */
     PyTypeObject *metatype = _PyType_CalculateMetaclass(&PyType_Type, bases);
     if (metatype == NULL) {
         Py_DECREF(bases);
-        return NULL;
+        goto fail;
     }
     res = (PyHeapTypeObject*)metatype->tp_alloc(metatype, nmembers);
     if (res == NULL) {
         Py_DECREF(bases);
-        return NULL;
+        goto fail;
     }
     res_start = (char*)res;
 
@@ -3614,7 +3613,7 @@ PyType_FromModuleAndSpec(PyObject *module, PyType_Spec *spec, PyObject *bases)
     return (PyObject*)res;
 
  fail:
-    Py_DECREF(res);
+    Py_XDECREF(res);
     return NULL;
 }