You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
PEP 697 -- Limited C API for Extending Opaque Types #103509
Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.
I'll just post my suggestion here instead of opening a PR. It kind of bothers me a bit that PyObject_GetTypeData and PyType_GetTypeDataSize currently always succeeds (and thus never raise an exception), but both are documented to raise an exception on error. I feel the urge to assert the current behaviour so we suddenly don't end up with them returning NULL or -1 without an exception set. What do you think, Petr, is it worth it?
suggestion - diff
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index 171c76a59a..ad7b9c1ce4 100644
--- a/Objects/typeobject.c+++ b/Objects/typeobject.c@@ -4532,7 +4532,12 @@ void *
PyObject_GetTypeData(PyObject *obj, PyTypeObject *cls)
{
assert(PyObject_TypeCheck(obj, cls));
- return (char *)obj + _align_up(cls->tp_base->tp_basicsize);+ void *ret = (char *)obj + _align_up(cls->tp_base->tp_basicsize);++ /* This API is documented to raise an exception if it returns NULL.+ * The current implementation, however, always succeeds. */+ assert(ret != NULL);+ return ret;
}
Py_ssize_t
@@ -4542,6 +4547,10 @@ PyType_GetTypeDataSize(PyTypeObject *cls)
if (result < 0) {
return 0;
}
++ /* This API is documented to raise an exception if it returns -1.+ * The current implementation, however, always succeeds. */+ assert(result >= 0);
return result;
}
It doesn't bother me. (char *)obj + _align_up(cls->tp_base->tp_basicsize) can obviously never be NULL, so the assert is superfluous. If a future dev sets it to something that can be NULL, they're responsible for also setting an exception -- as always when adding an error case. The fact that there's no existing error case looks irrelevant to me.
We could add a comment for clarity, but I would word it as.
/* The user must ensure that *cls* was created using
negative PyType_Spec.basicsize. Currently we don't have a good way to check that.
For errors that we can detect, this function should raise an exception and return NULL. */
However, that's just repeating what's already in the docs...
Activity
pythongh-103509: PEP 697 -- Limited C API for Extending Opaque Types
gh-103509: PEP 697 -- Limited C API for Extending Opaque Types (GH-10…
encukou commentedon May 4, 2023
Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.
erlend-aasland commentedon May 4, 2023
I'll just post my suggestion here instead of opening a PR. It kind of bothers me a bit that
PyObject_GetTypeDataandPyType_GetTypeDataSizecurrently always succeeds (and thus never raise an exception), but both are documented to raise an exception on error. I feel the urge to assert the current behaviour so we suddenly don't end up with them returningNULLor-1without an exception set. What do you think, Petr, is it worth it?suggestion - diff
sunmy2019 commentedon May 4, 2023
makes sense
encukou commentedon May 4, 2023
It doesn't bother me.
(char *)obj + _align_up(cls->tp_base->tp_basicsize)can obviously never be NULL, so theassertis superfluous. If a future dev sets it to something that can be NULL, they're responsible for also setting an exception -- as always when adding an error case. The fact that there's no existing error case looks irrelevant to me.We could add a comment for clarity, but I would word it as.
However, that's just repeating what's already in the docs...
erlend-aasland commentedon May 4, 2023
NP, it's not a deal-breaker for me. Let's just leave it as it is 🙂