Sorry, something went wrong.
There was a problem hiding this comment.
Maybe you can use PyModule_AddType in here too?
Sorry, something went wrong.
There was a problem hiding this comment.
@shihai1991 yes and I guess I should convert these to heap types too
Sorry, something went wrong.
There was a problem hiding this comment.
@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:
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe you can take a look of victor's PR: 1c2fa78
Sorry, something went wrong.
There was a problem hiding this comment.
@shihai1991 That PR does not have any types with an as_mapping method defined
Sorry, something went wrong.
There was a problem hiding this comment.
If it is not possible to subclass the type, you can use PyType_GetModule().
Sorry, something went wrong.
There was a problem hiding this comment.
@vstinner I think I'm missing some information. How can I use PyType_GetModule() on a method signature like:
There is no type here for me to access.
Sorry, something went wrong.
There was a problem hiding this comment.
Py_TYPE(self) gives you a type.
Sorry, something went wrong.
There was a problem hiding this comment.
@vstinner oh, right. Every object struct starts with PyObject_HEAD which has the type. I totally forgot about that.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
@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?
Sorry, something went wrong.
There was a problem hiding this comment.
@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.
Sorry, something went wrong.
There was a problem hiding this comment.
Usually to convert an extension module to multiphase init, you must convert static types to heap types first.
Sorry, something went wrong.
There was a problem hiding this comment.
@vstinner see my comment in response to @shihai1991 about this - I was not able to convert these to heap types
Sorry, something went wrong.
There was a problem hiding this comment.
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:
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).
Sorry, something went wrong.
There was a problem hiding this comment.
@vstinner I am more comfortable with module state, anyways eventually it will need it if we migrate to heap types.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Same here: It is a test module that does not require changes. |
Sorry, something went wrong.
@vstinner @shihai1991 please review
This is the other half of #21986
https://bugs.python.org/issue1635741