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: convert unicode ucd type to heap type by koubaa · Pull Request #22490 · python/cpython Conversation
|
|
@vstinner 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.
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.
|
While this change is correct, I'm not excited by leaking a new heap type at Python exit. Would you mind to attempt to add a module state to the module, and pass to all functions which use the UCD_Type? (without converting the UCD_Type to a heap type) The module state can be an empty structure, or add a "int dummy" just to make it non-empty. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@vstinner This is going to a large set of changes, almost as large as the original PR, because I have to introduce an additional layer for all methods which are used by both the module and the type. I don't see how this will help with the leak |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
2862eea to
d612585
Compare
|
This PR is outdated, unicodedata got many changes in the meanwhile. I proposed one approach to convert unicodedata to multi-phase init in https://bugs.python.org/issue42157 I close this PR. Once PR #22990 will be merged, I will propose a PR to finally convert the module to multi-phase init. Sorry for the misunderstanding, but this extension module is way more complex than other extensions, and I didn't spot all corner cases at the first review. See the bpo for the list of all issues and my proposal. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@vstinner no problem, I agree it is complex. |
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.