FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-1635741: convert unicode ucd type to heap type by koubaa · Pull Request #22490 · python/cpython · GitHub

/ cpython Public

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

Closed
koubaa wants to merge 2 commits into
python:masterfrom
koubaa:bpo-1635741-unicode-heap-type
Closed

bpo-1635741: convert unicode ucd type to heap type#22490
koubaa wants to merge 2 commits into
python:masterfrom
koubaa:bpo-1635741-unicode-heap-type

Conversation

koubaa commented Oct 2, 2020
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

koubaa commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner please review

Comment thread Modules/unicodedata.c Outdated
Comment thread Modules/unicodedata.c Outdated
Comment thread Modules/unicodedata.c Outdated
Comment thread Modules/unicodedata.c Outdated
Comment thread Modules/unicodedata.c Outdated

Copy link
Copy Markdown
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 commented Oct 12, 2020

Copy link
Copy Markdown
Contributor Author

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 force-pushed the bpo-1635741-unicode-heap-type branch from 2862eea to d612585 Compare October 19, 2020 01:53

Copy link
Copy Markdown
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 closed this Oct 26, 2020

koubaa commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner no problem, I agree it is complex.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL