Repository navigation
PEP 697 -- Limited C API for Extending Opaque Types #103509
Description
Activity
Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.
Reacted by Erlend E. Aasland and Oleg IaryginErlend 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_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
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; }
I feel the urge to assert the current behaviour
makes sense
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.
/* 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...
NP, it's not a deal-breaker for me. Let's just leave it as it is 🙂
PEP 697 was accepted and needs to be implemented.
Linked PRs