bpo-1635741: port pyexpat to multi-phase init (PEP 489) by koubaa · Pull Request #22222 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation46 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
@vstinner @corona10 @shihai1991 please review.
There's PyCapsule_New usage but the capsule methods did not depend on any globals so it isn't a concern in this case.
compile warning in here:
##[warning]/home/runner/work/cpython/cpython/Modules/pyexpat.c:1185:1: warning: ‘xmlparse_dealloc’ defined but not used [-Wunused-function]
xmlparse_dealloc(xmlparseobject *self)
^~~~~~~~~~~~~~~~
compile warning in here:
##[warning]/home/runner/work/cpython/cpython/Modules/pyexpat.c:1185:1: warning: ‘xmlparse_dealloc’ defined but not used [-Wunused-function] xmlparse_dealloc(xmlparseobject *self) ^~~~~~~~~~~~~~~~
Oops, I forgot to add this to the type slots. Should be fixed now
| } |
|---|
| Py_INCREF(state->error); |
| if (PyModule_AddObject(mod, "error", state->error ) < 0) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DECREF is needed on error, no? Same comment for the following PyModule_AddObject calls.
| } |
|---|
| errors_module = PyDict_GetItemWithError(d, errmod_name); |
| PyObject *errors_module = PyDict_GetItemWithError(d, errmod_name); |
| if (errors_module == NULL && !PyErr_Occurred()) { |
| errors_module = PyModule_New(MODULE_NAME ".errors"); |
| if (errors_module != NULL) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to "goto error" if PyModule_New() fails.
Maybe it would be simpler to extract changes which add error handling to the exec function, and write a second PR to convert the module to multi-phase init. Since the existing code basically has no code to handle errors :-(
You can rename pyexpat_exec() to PyInit_pyexpat() and just add Py_DECREF(m) in "goto error".
I would prefer to get PR #22489 merged before this one, to ease review ;-)
@vstinner rebased and I think ready to go. Please review
Shouldn't handler_info also be a part of the module state?
@shihai1991 made expat_CAPI per module instance in commit 7c83eaa.
I merged the PR, thanks @koubaa!
Shouldn't handler_info also be a part of the module state?
Hum. It doesn't contain anything dynamically allocated. It's a static array of static data. I don't think that we have to make it per instance.
adorilson pushed a commit to adorilson/cpython that referenced this pull request
