On Tue, Oct 27, 2015 at 9:34 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Hi > > 2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcatalin@gmail.com>: >> The current code doesn't build on Python3 because the 3rd argument of >> PyMethod_New, the troubled one you need set to NULL has been removed. >> This has to do with the distinction between bound and unbound methods >> which is gone in Python3. > > > this part of is well isolated, and we can do switch for Python2 and Python3 > without significant problems.
I had a quick look at this and at least 2 things are needed for Python3:
* in the PLy_spi_error_methods definition, __init__ has to take METH_VARARGS | METH_KEYWORDS not just METH_KEYWORDS; in Python2 METH_KEYWORDS implied METH_VARARGS so it's not needed (it also doesn't hurt) but if I don't add it, in Python3 I get: ERROR: SystemError: Bad call flags in PyCFunction_Call. METH_OLDARGS is no longer supported! >> Still, the above link shows a (more verbose but maybe better) >> alternative: don't use PyErr_NewException and instead define an >> SPIError type with each slot spelled out explicitly. This will remove >> the "is it safe to set the third argument to NULL" question. > > Should be there some problems, if we lost dependency on basic exception > class? Some compatibility issues? Or is possible to create full type with > inheritance support?
It's possible to give it a base type, see at https://hg.python.org/cpython/rev/429cadbc5b10/ this line before calling PyType_Ready: PyComError_Type.tp_base = (PyTypeObject*)PyExc_Exception;
PyErr_NewException is a shortcut for defining simple Exception deriving types, usually one defines a type with the full PyTypeObject definition.
In the meantime, I had a deeper look at the 2.7.10 code and I trust that PyMethod_New with the last argument set to NULL is ok. Setting that to NULL will lead to the PyMethod representing __init__ im_class being NULL. But that PyMethod object is not held onto by C code, it's added to the SPIError class' dict. From there, it is always retrieved from Python via an instance or via the class (so SPIError().__init__ or SPIError.__init__) which will lead to instancemethod_descr_get being called because it's the tp_descr_get slot of PyMethod_Type and that code knows the class you're retrieving the attribute from (SPIError in this case), checks if the PyMethod already has a not NULL im_class (which SPIError.__init__ doesn't) and, if not, calls PyMethod_New again and passes the class (SPIError) as the 3rd argument.
Given this, I think it's ok to keep using PyErr_NewException rather than spelling out the type explicitly.
Thank you very much for your analyse. I am sending new version of proposed patch with Python3 support. Fixed missing check of dictionary initialization.
