bpo-37151: remove special case for PyCFunction from PyObject_Call #14684
Conversation
jdemeyer
force-pushed
the
PyObject_Call_cfunction
branch
from
567d118 to
e0f6c09
Compare
|
|
||
| assert(PyCFunction_GET_FLAGS(func) & METH_VARARGS); | ||
| if (PyCFunction_GET_FLAGS(func) & METH_KEYWORDS) { | ||
| if (Py_EnterRecursiveCall(" while calling a Python object")) { |
There was a problem hiding this comment.
The recursion check is gone now. Was that intentional? Any reason why you think we don't need it any more?
Since you're dealing with a public C-API function here, I would advise to keep the current semantics.
There was a problem hiding this comment.
The recursion check is gone from PyCFunction_Call but it is in PyObject_Call.
Note that PyCFunction_Call is not a documented C API function, so I assumed that this minor change in behaviour (no longer doing the recursion check) would be acceptable.
That's the kind of function that projects like Cython call directly. |
What point are you trying to make? It that just a comment or is it saying something about this PR? Or are you just complaining about Cython? Or is it a joke somehow? |
|
I suggest to ensure that any PyCFunction_Call behavior change will not break anything, especially in Cython. |
|
It is undocumented, so the only documentation available to people who wanted to use this function was the code itself. Perhaps CPython could use |
|
I checked and Cython doesn't use |
At that point, probably best would be to just alias it to |
|
Right, it's quite useless as public API. Remove it in 3.9. But:
It was probably never intended to be public (and probably predates discussions on what should be public), but it is an exported symbol. |
|
We need to keep it for the stable ABI, but that's easily solved by making it an alias of I updated my branch to do this. |
|
Added a deprecation for |
| @@ -39,7 +39,7 @@ PyAPI_FUNC(int) PyCFunction_GetFlags(PyObject *); | |||
| #define PyCFunction_GET_FLAGS(func) \ | |||
| (((PyCFunctionObject *)func) -> m_ml -> ml_flags) | |||
| #endif | |||
| PyAPI_FUNC(PyObject *) PyCFunction_Call(PyObject *, PyObject *, PyObject *); | |||
| Py_DEPRECATED(3.9) PyAPI_FUNC(PyObject *) PyCFunction_Call(PyObject *, PyObject *, PyObject *); | |||
There was a problem hiding this comment.
@vstinner, is this the right way to deprecate an undocumented part of the stable ABI?
(I'll merge regardless, but I wanted to bring this to your attention.)
There was a problem hiding this comment.
The stable ABI is supposed to remain unchanged :-) "Py_DEPRECATED(3.9) PyAPI_FUNC(PyObject *) PyCFunction_Call" is the right usage of Py_DEPRECATED().
I don't know why PyCFunction_Call "leaked" into the stable ABI. I'm in favor of removing it. Start with a deprecation is a good start ;)
…thonGH-14684) bpo-37151: remove special case for PyCFunction from PyObject_Call Alse, make the undocumented function PyCFunction_Call an alias of PyObject_Call and deprecate it.
…thonGH-14684) bpo-37151: remove special case for PyCFunction from PyObject_Call Alse, make the undocumented function PyCFunction_Call an alias of PyObject_Call and deprecate it.
The function
PyObject_Callcontains a special case for calling instances ofPyCFunctionwithMETH_VARARGS. This is to avoid a double recursion check:cfunction_call_varargsalready callsPy_EnterRecursiveCall, soPyObject_Callshould not. We can simplify this by removing the recursion check fromcfunction_call_varargsand removing the special case fromPyObject_Call.In addition, we fold
cfunction_call_varargsintoPyCFunction_Calland move it back toObjects/methodobject.cwhich already contains the vectorcall functions ofPyCFunction.https://bugs.python.org/issue37151