Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-1635741: port pyexpat to multi-phase init (PEP 489)#22222
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
koubaa commented Sep 13, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
koubaa commented Sep 13, 2020
@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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
shihai1991 commented Sep 13, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
compile warning in here: |
Uh oh!
There was an error while loading. Please reload this page.
koubaa commented Sep 13, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Oops, I forgot to add this to the type slots. Should be fixed now |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Modules/pyexpat.c Outdated
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Modules/pyexpat.c Outdated
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Sep 23, 2020
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 commented Oct 2, 2020
I would prefer to get PR #22489 merged before this one, to ease review ;-) |
bf06df6 to c483618Comparekoubaa commented Nov 7, 2020
@vstinner rebased and I think ready to go. Please review |
Uh oh!
There was an error while loading. Please reload this page.
c483618 to 5262887Compareerlend-aasland commented Jan 4, 2021
Shouldn't |
vstinner commented Jan 4, 2021
@shihai1991 made expat_CAPI per module instance in commit 7c83eaa. I merged the PR, thanks @koubaa!
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. |
https://bugs.python.org/issue1635741