Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-35134: Add Include/cpython/pytime.h file#23988
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nw0 commented Dec 29, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
Add Include/cpython/pytime.h header file. Move CPython C API from Include/pytime.h into a new Include/cpython/pytime.h header file, which is included by Include/pytime.h.
nw0 commented Dec 29, 2020
Hmm, I'd like to add the skip-news label like previous PRs for this issue, but can't seem to do this. Help? |
vstinner commented Jan 20, 2021
Hum, since the new Include/pytime.h is basically empty. I suggest to leave the file unchanged and the move it into Include/cpython/. I don't expect anyone to include it directly, and it's already included by "Python.h". The C API documentation asks to only include "Python.h." |
Address review; replaced by Include/cpython/pytime.h
| #include"pycore_pyerrors.h" | ||
| #include"pycore_pystate.h"// _PyThreadState_GET() | ||
| #include"pydtrace.h" | ||
| #include"pytime.h"// _PyTime_GetMonotonicClock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum. Since it's included by Python.h, including it explicitly is not needed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated these, and moved the pytime.h include further down in Python.h to allow for it. I believe there aren't any other files including pytime.h directly now.
Include/cpython/pytime.h Outdated
| #include"pyconfig.h"/* include for defines */ | ||
| #include"object.h" | ||
| #include"../object.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that people should include pytime.h directly, and so I suggest to simply remove these includes. Let people respect the Python C API documentation which suggests to only include "Python.h".
vstinner commented Feb 16, 2021
Thank you, I merged your PR. |
bedevere-bot commented Feb 16, 2021
|
This change is backward compatible since C extension modules must not include "pytime.h" directly, but only include "Python.h".
Add Include/cpython/pytime.h header file.
Move CPython C API from Include/pytime.h into a new
Include/cpython/pytime.h header file, which is included by
Include/pytime.h.
https://bugs.python.org/issue35134
In the same vein as previous PRs by @vstinner. I've never done this before, so apologies in advance if I've messed anything up.
https://bugs.python.org/issue35134