Skip to content

bpo-40024: Add PyModule_AddType() helper function#19088

Merged
vstinner merged 6 commits intopython:masterfrom
corona10:bpo-40024
Mar 22, 2020
Merged

bpo-40024: Add PyModule_AddType() helper function#19088
vstinner merged 6 commits intopython:masterfrom
corona10:bpo-40024

Conversation

@corona10
Copy link
Copy Markdown
Member

@corona10 corona10 commented Mar 20, 2020 •
edited by bedevere-bot
Loading

@corona10 corona10 requested a review from rhettinger as a code owner March 20, 2020 09:42
@corona10 corona10 requested a review from vstinner March 20, 2020 10:32
vstinner
vstinner reviewed Mar 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

IMHO such helper function deserves to be added to the limited C API. I know that I said the opposite a few days ago ;-) But now I can review its implementation and how it's used in practice ;-)

Modules/_weakref.c would also benefit of this helper function. Well, I get that almost all C extension would benefit of this helper. But let's start with the current PR + _weakref, and then write a second PR to convert all other C extension to PyModule_AddType() at once.

Comment thread Include/modsupport.h Outdated
@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Mar 21, 2020

@vstinner
Hey Victor, unfortunately, _weakref is not the case for this time.
For example _PyWeakref_RefType tp_name is weakref but on _weakref.c adds this type object as ref.
So I add the new case with _lzmamodule for this function.

@corona10 corona10 requested a review from vstinner March 21, 2020 14:26
vstinner
vstinner reviewed Mar 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Can you please rename _PyModule_AddType to PyModule_AddType() to make it public? Names starting with _Py are private functions.

Comment thread Doc/c-api/module.rst Outdated
Comment thread Misc/NEWS.d/next/C API/2020-03-20-18-41-33.bpo-40024.9zHpve.rst Outdated
@vstinner vstinner changed the title bpo-40024: Add _PyModule_AddType helper function. bpo-40024: Add PyModule_AddType() helper function Mar 21, 2020
vstinner
vstinner reviewed Mar 21, 2020
View reviewed changes
Comment thread Python/modsupport.c
Comment thread Modules/_collectionsmodule.c Outdated
@corona10 corona10 requested a review from vstinner March 22, 2020 06:48
vstinner
vstinner approved these changes Mar 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. Just a last request on the documentation.

Comment thread Doc/c-api/module.rst Outdated
@corona10 @vstinner
Update Doc/c-api/module.rst
0a29720 
Co-Authored-By: Victor Stinner <[email protected]>
@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Mar 22, 2020

@vstinner
Thanks for the review. I apply the suggestion!

@vstinner vstinner merged commit 05e4a29 into python:master Mar 22, 2020
@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 22, 2020

Merged, thanks.

@corona10 corona10 mentioned this pull request Mar 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@corona10 @vstinner @the-knights-who-say-ni @bedevere-bot