Skip to content

bpo-41798: Allocate unicodedata CAPI on the heap - #24128

Merged
vstinner merged 3 commits into
python:masterfrom
erlend-aasland:bpo-41798/unicodedata
Jan 20, 2021
Merged

bpo-41798: Allocate unicodedata CAPI on the heap#24128
vstinner merged 3 commits into
python:masterfrom
erlend-aasland:bpo-41798/unicodedata

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Jan 5, 2021•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@erlend-aasland

erlend-aasland commented Jan 5, 2021

Copy link
Copy Markdown
ContributorAuthor

cc @shihai1991

shihai1991
shihai1991 reviewed Jan 6, 2021
View reviewed changes
Comment threadModules/unicodedata.c Outdated
PyMem_Free(capi);
return -1;
}
if (PyModule_AddObject(module, "_ucnhash_CAPI", capsule) < 0) {

@shihai1991shihai1991Jan 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using PyModule_AddObjectRef in here too?

@erlend-aaslanderlend-aaslandJan 6, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that, but I'm not sure it would improve this code. What do you think?

@shihai1991shihai1991Jan 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyModule_AddObjectRef() is more easy to understand than PyModule_AddObject(), because PyModule_AddObject() will steal the refs sometimes.

@erlend-aaslanderlend-aaslandJan 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it, I do agree. It's easier to follow the ref count when reading the code. I'll change it. Thanks!

@erlend-aaslanderlend-aaslandJan 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change the other PyModule_AddObject in unicodedata_exec() while we're there?

@erlend-aasland

erlend-aasland commented Jan 7, 2021

Copy link
Copy Markdown
ContributorAuthor

@vstinner, would you mind reviewing this?

@vstinner

vstinner commented Jan 8, 2021

Copy link
Copy Markdown
Member

@vstinner, would you mind reviewing this?

Not yet. I'm fixing the second regression caused by your latest change :-D https://bugs.python.org/issue42866 I'm not blaming you, regressions are common, and I'm fine with dealing with them. It's just that I prefer to fix all known regressions before taking the risk of adding new ones ;-)

@erlend-aasland

erlend-aasland commented Jan 8, 2021

Copy link
Copy Markdown
ContributorAuthor

Not yet. I'm fixing the second regression caused by your latest change :-D https://bugs.python.org/issue42866 I'm not blaming you, regressions are common, and I'm fine with dealing with them. It's just that I prefer to fix all known regressions before taking the risk of adding new ones ;-)

I totally understand that, and I appreciate you helping out fixing the regressions :)

vstinner
vstinner reviewed Jan 20, 2021
View reviewed changes
Comment threadModules/unicodedata.c Outdated
Comment threadModules/unicodedata.c Outdated
Comment threadModules/unicodedata.c
@erlend-aasland

erlend-aasland commented Jan 20, 2021

Copy link
Copy Markdown
ContributorAuthor

PTAL, @vstinner

@vstinner
vstinner merged commit 61d2639 into python:masterJan 20, 2021
@vstinner

vstinner commented Jan 20, 2021

Copy link
Copy Markdown
Member

Merged, thanks.

@erlend-aasland
erlend-aasland deleted the bpo-41798/unicodedata branch January 20, 2021 11:04
@erlend-aasland

erlend-aasland commented Jan 20, 2021

Copy link
Copy Markdown
ContributorAuthor

Merged, thanks.

Thanks for reviewing!

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Erlend Egeberg Aasland@adorilson
bpo-41798: Allocate unicodedata CAPI on the heap (pythonGH-24128)
32e99a8
@stestaggstestagg mentioned this pull request May 6, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments
@shihai1991shihai1991shihai1991 left review comments

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland@vstinner@shihai1991@the-knights-who-say-ni@bedevere-bot