Skip to content

Comments

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

Merged
vstinner merged 8 commits intopython:mainfrom
vstinner:nanosleep
Sep 25, 2021
Merged

bpo-21302: Add nanosleep() implementation for time.sleep() in Unix#28545
vstinner merged 8 commits intopython:mainfrom
vstinner:nanosleep

Conversation

@vstinner
Copy link
Member

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

@vstinner
Copy link
Member Author

vstinner commented Sep 24, 2021

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

vstinner commented Sep 24, 2021

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

Livius90 commented Sep 24, 2021

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

vstinner commented Sep 24, 2021

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

Livius90 commented Sep 25, 2021

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

vstinner commented Sep 25, 2021

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