[cython/cython] Make classmethod work in limited-api (PR #5800)
da-woods
Except for the case where binding=False, which I don't think we can easily do. I don't care that much given that binding=True works correctly.
Fixes #5797
You can view, comment on, or merge this pull request online at:
https://github.com/cython/cython/pull/5800
Commit Summary
- df54119 Make classmethod work in limited-api
File Changes
(2 files)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
da-woods
@da-woods commented on this pull request.
In Cython/Utility/CythonFunction.c:
> @@ -1792,13 +1795,54 @@ static PyObject* __Pyx_Method_ClassMethod(PyObject *method) {
PyTypeObject *d_type = descr->d_common.d_type;
#endif
return PyDescr_NewClassMethod(d_type, descr->d_method);
+#else
+ return PyErr_Format(
I suspect it would be possible to cover this case by setting METH_CLASS when the PyMethodDef is defined. I definitely don't think that's worth it for the first pass and may not be worth it in general.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Vyas Ramasubramani
@vyasr approved this pull request.
The implementation looks fine. I'm not sure if there are improvements that could be made but I do think it is correct.
In Cython/Utility/CythonFunction.c:
> else if (PyMethod_Check(method)) {
// python classes
return PyClassMethod_New(PyMethod_GET_FUNCTION(method));
}
else {
return PyClassMethod_New(method);
}
+#else
+ {
+ PyObject *types_module, *method_type=NULL, *func=NULL;
+ PyObject *builtins, *classmethod, *result=NULL;
+ types_module = PyImport_ImportModule("types");
+ if (!types_module) {
+ return NULL;
+ }
+ method_type = PyObject_GetAttrString(types_module, "MethodType");
+ if (!method_type) goto bad;
+ if (__Pyx_TypeCheck(method, method_type)) {
+ func = PyObject_GetAttrString(method, "__func__");
I'm sort of surprised that PyMethod_GetFunction isn't part of the limited API. I wonder if there's a plan to add it at some point. I had to write code like this in a couple of places and it made me wonder if getattr is really the intended way to access everything in limited API mode. I couldn't find any good alternatives, though.
In Cython/Utility/CythonFunction.c:
> + builtins = PyEval_GetBuiltins(); // borrowed + if (!builtins) goto bad;
Would it make sense to extract this logic into a macro? Every limited API branch is going to be repeating this pattern everywhere. I'm imagining something like
#define goto_if_null(var, expr, label) var=expr; if(!var) goto label;
...
goto_if_null(builtins, PyEval_GetBuiltins(), bad);
Having seen it written out I'm not as convinced of the benefit as I was in my head, but I'll leave the suggestion in case you find it helpful.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
da-woods
I'm going to treat this as low impact, unlikely to break anything, and merge it in the fairly near future unless I get any objections...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
da-woods
Merged #5800 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.