Make classmethod work in limited-api #5800
Conversation
Except for the class 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 cython#5797
| @@ -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( | |||
There was a problem hiding this comment.
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.
| method_type = PyObject_GetAttrString(types_module, "MethodType"); | ||
| if (!method_type) goto bad; | ||
| if (__Pyx_TypeCheck(method, method_type)) { | ||
| func = PyObject_GetAttrString(method, "__func__"); |
There was a problem hiding this comment.
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.
| builtins = PyEval_GetBuiltins(); // borrowed | ||
| if (!builtins) goto bad; |
There was a problem hiding this comment.
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.
|
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... |
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