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.

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

gh-111956: Add thread-safe one-time initialization. #111960

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Nov 10, 2023

The one-time initialization (_PyOnceFlag) is used in two places:

  • Python/Python-ast.c
  • Python/getargs.c

The one-time initialization (`_PyOnceFlag`) is used in two places:

 * `Python/Python-ast.c`
 * `Python/getargs.c`
@colesbury
Copy link
Contributor Author

@ericsnowcurrently, when you have some time, would you please look at this?

One question: I'm unsure of what int value should be used here to indicate an error or success. I have gone with 0=error and 1=success here because that requires fewer modifications to the use sites, which already use that convention. I am unsure if -1=error, 0=success would be better.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Aside from one small thing, LGTM.

Python/getargs.c Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Nov 15, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ericsnowcurrently
Copy link
Member

I'm unsure of what int value should be used here to indicate an error or success. I have gone with 0=error and 1=success here because that requires fewer modifications to the use sites, which already use that convention. I am unsure if -1=error, 0=success would be better.

The typical convention in the case of success vs. error is -1 for error and 0 for success. The 0/1 convention is more for true/false situations.

@colesbury
Copy link
Contributor Author

@ericsnowcurrently that makes sense. I'll change it to -1=error, 0=success.

@colesbury
Copy link
Contributor Author

@ericsnowcurrently, I changed the code to use -1=error 0=success and updated asdl_c.py/Python-ast.c. It ended up being a large change to that file, but fixed one bug where we ignored errors in init_identifiers due to a mismatch in return code assumptions.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

FWIW, I haven't combed through the asdl_c.py changes but would expect the test suite to catch any mistakes in there. It's also a good sign that you caught the bug in init_identifiers(). 😄

@ericsnowcurrently
Copy link
Member

I'll merge this in the morning if no one beats me to it.

@ericsnowcurrently ericsnowcurrently merged commit 446f18a into python:main Nov 16, 2023
33 of 34 checks passed
@colesbury colesbury deleted the once branch December 12, 2023 19:38
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024