GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||
GeezerVison is showing github.com — bpo-1635741: port pyexpat to multi-phase init (PEP 489) by koubaa · Pull Request #22222 · python/cpython Conversation
|
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
compile warning in here: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Oops, I forgot to add this to the type slots. Should be fixed now |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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". |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I would prefer to get PR #22489 merged before this one, to ease review ;-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
bf06df6 to
c483618
Compare
|
@vstinner rebased and I think ready to go. Please review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
c483618 to
5262887
Compare
|
Shouldn't |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.