Skip to content

bpo-1635741: port pyexpat to multi-phase init (PEP 489) - #22222

Merged
vstinner merged 3 commits into
python:masterfrom
koubaa:bpo-1635741-pyexpat
Jan 4, 2021
Merged

bpo-1635741: port pyexpat to multi-phase init (PEP 489)#22222
vstinner merged 3 commits into
python:masterfrom
koubaa:bpo-1635741-pyexpat

Conversation

@koubaa

@koubaakoubaa commented Sep 13, 2020

Copy link
Copy Markdown
Contributor

@koubaa

Copy link
Copy Markdown
ContributorAuthor

@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.

Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
@shihai1991

shihai1991 commented Sep 13, 2020

Copy link
Copy Markdown
Member

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)
^~~~~~~~~~~~~~~~

Comment threadModules/pyexpat.c Outdated
@koubaa

koubaa commented Sep 13, 2020

Copy link
Copy Markdown
ContributorAuthor

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

Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated

Copy link
Copy Markdown
Member

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.

Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated

Copy link
Copy Markdown
Member

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.

Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
Comment threadModules/pyexpat.c Outdated
@vstinner

Copy link
Copy Markdown
Member

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".

@vstinner

Copy link
Copy Markdown
Member

I would prefer to get PR #22489 merged before this one, to ease review ;-)

@koubaa
koubaaforce-pushed the bpo-1635741-pyexpat branch from bf06df6 to c483618CompareNovember 7, 2020 02:07
@koubaa

Copy link
Copy Markdown
ContributorAuthor

@vstinner rebased and I think ready to go. Please review

Comment threadModules/pyexpat.c Outdated
@erlend-aasland

Copy link
Copy Markdown
Contributor

Shouldn't handler_info also be a part of the module state?

@vstinner
vstinner merged commit c8a87ad into python:masterJan 4, 2021
@vstinner

Copy link
Copy Markdown
Member

@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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@koubaa@shihai1991@vstinner@erlend-aasland@the-knights-who-say-ni@bedevere-bot