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

@koubaa koubaa commented Sep 13, 2020

Copy link
Copy Markdown
Contributor
@koubaa

koubaa commented Sep 13, 2020

Copy link
Copy Markdown
Contributor Author

@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 thread Modules/pyexpat.c Outdated
Comment thread Modules/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 thread Modules/pyexpat.c Outdated
@koubaa

koubaa commented Sep 13, 2020

Copy link
Copy Markdown
Contributor Author

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 thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/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 thread Modules/pyexpat.c Outdated
Comment thread Modules/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 thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/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

vstinner commented Oct 2, 2020

Copy link
Copy Markdown
Member

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

@koubaa
koubaa force-pushed the bpo-1635741-pyexpat branch from bf06df6 to c483618 Compare November 7, 2020 02:07
@koubaa

koubaa commented Nov 7, 2020

Copy link
Copy Markdown
Contributor Author

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

Comment thread Modules/pyexpat.c Outdated
@koubaa
koubaa force-pushed the bpo-1635741-pyexpat branch from c483618 to 5262887 Compare November 15, 2020 17:32
@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:master Jan 4, 2021
@vstinner

vstinner commented Jan 4, 2021

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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants