Skip to content

bpo-46417: Add _PyType_CAST() macro #30760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 21, 2022
Merged

bpo-46417: Add _PyType_CAST() macro #30760

merged 1 commit into from
Jan 21, 2022

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 21, 2022

In debug mode, the macro makes sure that its argument is a type using
an assertion.

https://bugs.python.org/issue46417

In debug mode, the macro makes sure that its argument is a type using
an assertion.
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you left some ordinary type casts. Was that deliberate, or did you miss some _PyType_CASTs?

if (!PyType_Check(ob)) {
PyErr_Format(PyExc_TypeError,
"%s.__bases__ must be tuple of classes, not '%s'",
type->tp_name, Py_TYPE(ob)->tp_name);
return -1;
}
PyTypeObject *base = (PyTypeObject*)ob;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use _PyType_CAST here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just checked PyType_Check() at runtime (even if assertions are removed by the compiler), using the macro is redundant (it would call PyType_Check() twice in debug mode).

return -1;
}
PyTypeObject *base = (PyTypeObject*)obj;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

if (!PyType_Check(base_proto)) {
PyErr_SetString(
PyExc_TypeError,
"bases must be types");
return NULL;
}
base_i = (PyTypeObject *)base_proto;
PyTypeObject *base_i = (PyTypeObject *)base_proto;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -4793,12 +4780,13 @@ object_set_class(PyObject *self, PyObject *value, void *closure)
Py_TYPE(value)->tp_name);
return -1;
}
PyTypeObject *newto = (PyTypeObject *)value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

PyObject *arg0, *res;

if (self == NULL || !PyType_Check(self)) {
PyErr_Format(PyExc_SystemError,
"__new__() called with non-type 'self'");
return NULL;
}
type = (PyTypeObject *)self;
PyTypeObject *type = (PyTypeObject *)self;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@@ -7086,7 +7077,8 @@ tp_new_wrapper(PyObject *self, PyObject *args, PyObject *kwds)
Py_TYPE(arg0)->tp_name);
return NULL;
}
subtype = (PyTypeObject *)arg0;
PyTypeObject *subtype = (PyTypeObject *)arg0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

if ((PyObject *)subclass == Py_None)
PyObject *obj = PyWeakref_GET_OBJECT(ref);
assert(obj != NULL);
if (obj == Py_None) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (obj == Py_None) {
if (Py_IsNone(obj)) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trust me, the temptation to refactor everything in this old giant C file is very high :-D But then it leads to PR which is impossible to review :-( I prefer to leave this test unchanged.

@vstinner
Copy link
Member Author

I see you left some ordinary type casts. Was that deliberate, or did you miss some _PyType_CASTs?

I left unchanged the raw type casts which are just after a PyType_Check() test.

Locally, I have a local Git branch to cleanup way more casts:

 Modules/_csv.c                     | 20 ++++++++++----------
 Modules/_datetimemodule.c          |  8 ++++----
 Modules/_pickle.c                  | 20 +++++++++++---------
 Modules/_randommodule.c            | 11 ++++++-----
 Modules/_sqlite/connection.c       |  5 +++--
 Modules/_sqlite/cursor.c           |  5 +++--
 Modules/_sqlite/prepare_protocol.c |  5 +++--
 Modules/_sqlite/row.c              |  5 +++--
 Modules/_sqlite/statement.c        |  5 +++--
 Modules/_testcapimodule.c          | 12 ++++++------
 Modules/_zoneinfo.c                |  3 +--
 Objects/complexobject.c            |  2 +-
 Objects/dictobject.c               |  3 +--
 Objects/enumobject.c               |  7 ++-----
 Objects/exceptions.c               |  3 +--
 Objects/floatobject.c              |  2 +-
 Objects/listobject.c               |  3 +--
 Objects/setobject.c                |  8 ++++----
 Objects/structseq.c                |  3 +--
 Objects/tupleobject.c              |  2 +-
 Objects/typeobject.c               | 65 ++++++++++++++++++++++++++++++-----------------------------------
 Python/bltinmodule.c               |  4 ++--
 Python/ceval.c                     | 22 +++++++++++-----------
 Python/specialize.c                | 13 ++++++-------
 24 files changed, 115 insertions(+), 121 deletions(-)

I prefer to create a separated PR for these ones :-)

@erlend-aasland
Copy link
Contributor

I left unchanged the raw type casts which are just after a PyType_Check() test.

Ok, I assumed you had a good reason for it :)

@vstinner vstinner merged commit bc67f18 into python:main Jan 21, 2022
@vstinner vstinner deleted the type_cast branch January 21, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants