Skip to content

bpo-1635741: convert unicode ucd type to heap type #22490

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

Closed
wants to merge 2 commits into from

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Oct 2, 2020

@koubaa
Copy link
Contributor Author

koubaa commented Oct 2, 2020

@vstinner please review

@vstinner
Copy link
Member

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.

@koubaa
Copy link
Contributor Author

koubaa commented Oct 12, 2020

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.

@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

@koubaa koubaa force-pushed the bpo-1635741-unicode-heap-type branch from 2862eea to d612585 Compare October 19, 2020 01:53
@vstinner
Copy link
Member

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.

@vstinner vstinner closed this Oct 26, 2020
@koubaa
Copy link
Contributor Author

koubaa commented Nov 2, 2020

@vstinner no problem, I agree it is complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants