bpo-1635741 port _testbuffer to multi-phase init

[koubaa]

@vstinner @shihai1991 please review

This is the other half of #21986

https://bugs.python.org/issue1635741

[shihai1991]

Maybe you can use PyModule_AddType in here too?

[koubaa]

@shihai1991 yes and I guess I should convert these to heap types too

[koubaa]

@shihai1991 @vstinner I am trying to convert the NDArray_Type to a heap type but it has binary functions in its as_mapping that need access to the module state, e.g.:

nd = (NDArrayObject *)ndarray_new(&NDArray_Type, NULL, NULL);

The signature of this method is:

static PyObject *
ndarray_subscript(NDArrayObject *self, PyObject *key)

There doesn't seem to be a way to get the module in this case - so I believe converting to heap types might block until a future PEP. Please correct me if I am wrong but I will proceed with using PyModule_AddType without heap types.

[shihai1991]

Maybe you can take a look of victor's PR: 1c2fa78

[koubaa]

@shihai1991 That PR does not have any types with an as_mapping method defined

[vstinner]

If it is not possible to subclass the type, you can use PyType_GetModule().

[koubaa]

@vstinner I think I'm missing some information. How can I use PyType_GetModule() on a method signature like:

static PyObject *
ndarray_subscript(NDArrayObject *self, PyObject *key)

There is no type here for me to access.

[vstinner]

Py_TYPE(self) gives you a type.

[koubaa]

@vstinner oh, right. Every object struct starts with PyObject_HEAD which has the type. I totally forgot about that.

[vstinner]

I may be simpler to have one PR for _testbuffer, and another for overlapped. These changes are really complex, so it's hard to review hard. It's simpler when a PR is on a single extension module. But you can also keep the PR as it is. It's up to you.

cc @corona10 @shihai1991

[vstinner]

Hum, would it be possible to move these static variables to be able to clear them when the extension module is unloaded? It would require to pass the module to NDArray_Type creation, so be able to get the module state from a ndarray instance.

static const char *simple_fmt = "B";
static PyObject *simple_format = NULL;

[koubaa]

@vstinner This seems tricky. How do you recommend passing the module to a static type like NDArray_Type. I can't put the module instance on the type or it will be overridden each time we do _testbuffer_exec. Is there a way to do this in tp_new?

[koubaa]

@vstinner What if I just call PyUnicode_FromString every time it is used, since as you say this is just a test module. Struct and calcsize should be handled properly in any case.

[vstinner]

Usually to convert an extension module to multiphase init, you must convert static types to heap types first.

[koubaa]

@vstinner see my comment in response to @shihai1991 about this - I was not able to convert these to heap types

[vstinner]

Converting _testbuffer to multiphase init allows to create two instances of the module, but the C implementation uses global variables which is incompatible with that:

static PyObject *Struct = NULL;
static PyObject *calcsize = NULL;

You must first avoid these global variables. I suggest you to attempt to pass the module to functions using Struct and calcsize, and then get Struct and calcsize from the module. Either use PyObject_GetAttrString() which can slow down the module (which is ok, the module is only used for testing purpose), or use a module state (see my other comment about that).

[koubaa]

@vstinner I am more comfortable with module state, anyways eventually it will need it if we migrate to heap types.

[koubaa]

I converted to heap types but now tests are failing (seems to be due to the "mismatch between initializer element and format string" exception thrown by pack_single). I'm going to try to figure out what I did that causes this.

[vstinner]

I'm going to try to figure out what I did that causes this.

Good luck :-) Ping me when you consider that the PR is ready for a new review. You can write a first PR just to convert static types to heap types if you prefer, to ease debug, and to ease review.

[skrah]

Same here: It is a test module that does not require changes.