-
-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PEP 3121, 384 refactoring applied to _datetime module #59595
Comments
Too late for 3.3 |
Fixed _dealloc methods. Also: Init now returns the previously initialized module if available. |
Add Alexander Belopolsky to nosy list as maintainer of datetime module. |
+ Py_CLEAR(_datetimemodulestate(m)->PyDateTime_DateTimeType); Style nit: I would really store the module state pointer in a variable here, instead of repeating _datetimemodulestate(m) every line. +PyObject* _Get_State(struct PyModuleDef*); I'm not sure why a module should define such generic a function, and especially not without a "Py" prefix. Besides, review comments from bpo-15653 apply here. |
I have now included the changes that Antoine suggested. The _Get_State was used for debugging purposes and is, as I think, no longer necessary. However we have yet to find a solution for the decref inside the dealloc methods. |
Perhaps ask for advice on python-dev? |
For some reason there are no review links, so I'll review in this message. +typedef struct { Names exposed in public headers (datetime.h is a public header) should start with Py or _Py. Other offenders include _datetimemodule_state, _datetimemodule, _datetimemodulestate_global. I don't think these names need to be defined in the header file at all. +typedef struct { These are not forward declarations anymore. There is no need for PyDateTime_ prefix. Use names from PyDateTime_CAPI struct. |
I would like to split this issue to separate PEP-3121 changes from PEP-384. PEP-3121 state cleanup implementation is clearly an improvement "from a resource management point of view." On the other hand, I don't see much benefit for the datetime module from using a stable ABI. Unless I am missing something, PEP-384 is primarily benefiting 3rd party developers who distribute binary modules that should run under multiple Python versions. ABI stability is not a concern for the stdlib modules. On the other hand, the price for multi-version support is rather steep. Statically allocated types are more efficient. For example, with static types, PyDate_CheckExact() is a simple macro that expands into a dereference and a pointer comparison - a couple of instructions at the CPU level. On the other hand, with a proposed patch, it will involve a function call to locate the module (PyState_FindModule), followed by another function call to locate the state (PyModule_GetState) and several more dereferences that may lead to cache misses and other pessimizations. There is an important behavior change related to multiple interpreters. Currently dates created by different interpreters have the same type. With the proposed change they will have different types. I don't think this is desirable. In short, let's go in baby steps. Let's implement PEP-3121 cleanup first and start a debate on the role of PEP-384 in stdlib. |
I have updated the patch to work again with the current version of the _datetimemodule. Regarding the suggestion of separating PEP-3121 and PEP-384. It might be true that datetime and other modules do not benefit directly from PEP-384, however it is still a fact that the stdlib modules should be seen as a set of reference modules, that are all implemented in a way that complies with the implementation fo the xxmodules. Anyway I am planning to start to commit all of the open changes that I have created during my GSOC in the next few months. So a decision regarding this separation concern might be helpful. :-) |
On 10.12.2012 11:39, Robin Schreiber wrote:
Please use "_Py_" prefixes for private symbols you put in the header Question: What happens if PyModule_GetState() or PyState_FindModule() The current code will segfault in such a situation. Thanks,Marc-Andre Lemburg |
The patch needs to be converted to a github PR. |
@robinschreiber I'm closing this because, a decade on, the patch is very much out of date. Please create a new issue if you would like to pursue this. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: