-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-83714: Fix stat_nanosecond_timestamp() for 32-bit time_t #141069
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
Conversation
|
!buildbot x86 Debian |
|
🤖 New build scheduled with the buildbot fleet by @vstinner for commit 6e56143 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F141069%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
| { | ||
| #if SIZEOF_LONG >= 8 | ||
| #if SIZEOF_TIME_T == 4 | ||
| return PyLong_FromLongLong(sec * SEC_TO_NS + nsec); |
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.
What is the type of SEC_TO_NS? Should not it or sec be cast to long long?
We will get a warning also in the very unlikely case of more than 64-bit long long. Maybe use condition SIZEOF_TIME_T + 4 <= SIZEOF_LONG_LONG?
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.
What is the type of SEC_TO_NS?
It's a long long: it uses the LL suffix.
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.
We will get a warning also in the very unlikely case of more than 64-bit long long.
I don't see which kind of warning would be emitted if long long is larger than 64-bit?
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.
The same warning. If long long is 128-bit and time_t is 64-bit, then the condition (LLONG_MIN/SEC_TO_NS) <= sec && sec <= (LLONG_MAX/SEC_TO_NS - 1) will be tested, and it will be always true.
I do not know if Python is currently used on any platform with larger than 64-bit long long. This is hypothetical.
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.
Anyway, this would be just a warning. Not wrong code or missed optimization.
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.
If long long is 128-bit and time_t is 64-bit
Oh ok. Well, I expect many warnings if long long is larger than 64-bit, not just on these lines. We can revisit the code once such platform will exist :-)
serhiy-storchaka
left a comment
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.
LGTM. 👍
| { | ||
| #if SIZEOF_LONG >= 8 | ||
| #if SIZEOF_TIME_T == 4 | ||
| return PyLong_FromLongLong(sec * SEC_TO_NS + nsec); |
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.
Anyway, this would be just a warning. Not wrong code or missed optimization.
|
The warnings are still fixed with this PR on x86 Debian: https://buildbot.python.org/#/builders/1285/builds/531 |
Uh oh!
There was an error while loading. Please reload this page.