bpo-1635741: port pyexpat to multi-phase init (PEP 489) by koubaa · Pull Request #22222 · python/cpython
@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
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.
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.
