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-21302: Add nanosleep() implementation for time.sleep() in Unix #28545

Merged
merged 8 commits into from
Sep 25, 2021
Merged

bpo-21302: Add nanosleep() implementation for time.sleep() in Unix #28545

merged 8 commits into from
Sep 25, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 24, 2021 •
edited by bedevere-bot
Loading

@vstinner
Copy link
Member Author

I created this PR based on @Livius90's PR #28526 to propose a "Refactor" commit.

@vstinner vstinner mentioned this pull request Sep 24, 2021
Closed
@vstinner
Copy link
Member Author

The PR #28526 LGTM, but I propose to write the #ifdef dance differently and rename secs to timeout to make the code more readable. So I created this PR based on PR #28526. For the main code in Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS, I merged the 3 code paths to put if #ifdef in the Py_BEGIN_ALLOW_THREADS block.

@Livius90: Is it better? What do you think?

@Livius90
Copy link
Contributor

I can not say that the new Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS #ifdef section is better and more readable. I lost to see easily these have different err variables passing/checking. Any other new naming style is better, sure.

https://github.com/python/cpython/blob/d7a6cdd1711c5e55e81c37cf4acc173d9093d83e/Modules/timemodule.c#L2092

@vstinner
Copy link
Member Author

I lost to see easily these have different err variables passing/checking.

Ok, I moved err closer to the function call.

@Livius90
Copy link
Contributor

I think, It is much better, thanks.

@vstinner vstinner merged commit 7834ff2 into python:main Sep 25, 2021
@vstinner vstinner deleted the nanosleep branch September 25, 2021 12:36
@vstinner
Copy link
Member Author

Thanks @Livius90! I merged the PR based on your PR :-) I also updated the documentation.

@shankarunni shankarunni mannequin mentioned this pull request Nov 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

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
@vstinner @Livius90 @the-knights-who-say-ni @bedevere-bot