Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-1635741: Port itertools module to multiphase initialization. #19044

Merged
merged 4 commits into from
Mar 17, 2020
Merged

bpo-1635741: Port itertools module to multiphase initialization. #19044

merged 4 commits into from
Mar 17, 2020

Conversation

corona10
Copy link
Member

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

@corona10 corona10 requested a review from rhettinger as a code owner March 17, 2020 15:18
@corona10 corona10 force-pushed the bpo-1635741-itertools branch from 223b1c0 to ce22af3 Compare March 17, 2020 15:28
@corona10 corona10 force-pushed the bpo-1635741-itertools branch from ce22af3 to 6980d78 Compare March 17, 2020 15:32
@corona10 corona10 requested a review from vstinner March 17, 2020 16:11
@corona10
Copy link
Member Author

@shihai1991 Can you please take a look?

vstinner
vstinner approved these changes Mar 17, 2020
View reviewed changes
Copy link
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. But I will wait for @shihai1991 review ;-)

Modules/itertoolsmodule.c Outdated Show resolved Hide resolved
vstinner
vstinner reviewed Mar 17, 2020
View reviewed changes
Modules/itertoolsmodule.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

You forgot a typelist[i], I fixed it ;-)

shihai1991
shihai1991 approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

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

LGTM ;)

@corona10
Copy link
Member Author

You forgot a typelist[i], I fixed it ;-)

Thanks :)

@shihai1991
Copy link
Member

there is one thing to be confirmed: race condition of identifier in typelists can be avoid too? no potential risk?

@corona10
Copy link
Member Author

there is one thing to be confirmed: race condition of identifier in typelists can be avoid too? no potential risk?

Can you explain more concrete situation?

@vstinner
Copy link
Member

there is one thing to be confirmed: race condition of identifier in typelists can be avoid too? no potential risk?

Currently, the GIL magically protects C extensions against race conditions. But the GIL can be released when executing arbitrary code. I don't think that it's the case here.

@vstinner
Copy link
Member

vstinner commented Mar 17, 2020 •
edited
Loading

Static types initialized by PyType_Ready() are bad. Types allocated on the heap using PyType_FromSpec() are better:
https://pythoncapi.readthedocs.io/type_object.html

But this should be addressed in a separated change ;-)

@vstinner vstinner merged commit 514c469 into python:master Mar 17, 2020
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
@corona10 corona10 deleted the bpo-1635741-itertools branch February 15, 2024 01:41
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

@shihai1991 shihai1991 shihai1991 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.
5 participants
@corona10 @vstinner @shihai1991 @the-knights-who-say-ni @bedevere-bot