Hi 2015-10-23 7:34 GMT+02:00 Catalin Iacob <iacobcata...@gmail.com>:
> On Sat, Oct 17, 2015 at 8:18 PM, Pavel Stehule <pavel.steh...@gmail.com> > wrote: > > here is new patch > > > > cleaned, all unwanted artefacts removed. I am not sure if used way for > > method registration is 100% valid, but I didn't find any related > > documentation. > > Pavel, some notes about the patch, not a full review (yet?). > > In PLy_add_exceptions PyDict_New is not checked for failure. > > In PLy_spi_error__init__, kw will be NULL if SPIError is called with > no arguments. With the current code NULL will get passed to > PyDict_Size which will generate something like SystemError Bad > internal function call. This also means a test using just SPIError() > is needed. > It seems args should never be NULL by the way, if there are no > arguments it's an empty tuple so the other side of the check is ok. > > 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. > > There is http://bugs.python.org/issue1587 which discusses how to > replace the "third argument" functionality for PyMethod_New in > Python3. One of the messages there links to > http://bugs.python.org/issue1505 and > https://hg.python.org/cpython/rev/429cadbc5b10/ which has an example > very similar to what you're trying to do, rewritten to work in > Python3. But this is still confusing: note that the replaced code > *didn't really use PyMethod_New at all* as the removed line meth = > PyMethod_New(func, NULL, ComError); was commented out and the replaced > code used to simply assign the function to the class dictionary > without even creating a method. > 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. > > I tried to answer the "is it safe to use NULL for the third argument > of PyMethod_New in Python2?" question and don't have a definite answer > yet. If you look at the CPython code it's used to set im_class > (Objects/classobject.c) of PyMethodObject which is accessible from > Python. > But this code: > init = plpy.SPIError.__init__ > plpy.notice("repr {} str {} im_class {}".format(repr(init), str(init), > init.im_class)) > produces: > NOTICE: repr <unbound method SPIError.__init__> str <unbound method > SPIError.__init__> im_class <class 'plpy.SPIError'> > so the SPIError class name is set in im_class from somewhere. But this > is all moot if you use the explicit SPIError type definition. > 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? > > >> Any help is welcome > > I can work with you on this. I don't have a lot of experience with the > C API but not zero either. > It is very helpful for me - C API doesn't look difficult, but when I have to do some really low level work I am lost :( Regards Pavel
