FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-83714: Fix stat_nanosecond_timestamp() for 32-bit time_t by vstinner · Pull Request #141069 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-83714: Fix stat_nanosecond_timestamp() for 32-bit time_t - #141069

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:statx_sec_to_ns2
Nov 5, 2025
ConversationCommits (1)ChecksFiles changed
Merged

gh-83714: Fix stat_nanosecond_timestamp() for 32-bit time_t#141069
vstinner merged 1 commit into
python:mainfrom
vstinner:statx_sec_to_ns2

Conversation

vstinner commented Nov 5, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Nov 5, 2025
Closed

vstinner commented Nov 5, 2025

Copy link
Copy Markdown
Member Author

!buildbot x86 Debian

bedevere-bot commented Nov 5, 2025

Copy link
Copy Markdown

🤖 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: x86 Debian

The builders matched are:

  • x86 Debian Installed with X PR
  • x86 Debian Non-Debug with X PR

vstinner mentioned this pull request Nov 5, 2025
Merged
serhiy-storchaka reviewed Nov 5, 2025
View reviewed changes
Comment thread Modules/posixmodule.c
{
#if SIZEOF_LONG >= 8
#if SIZEOF_TIME_T == 4
return PyLong_FromLongLong(sec * SEC_TO_NS + nsec);

serhiy-storchaka Nov 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

vstinner Nov 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is the type of SEC_TO_NS?

It's a long long: it uses the LL suffix.

vstinner Nov 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

serhiy-storchaka Nov 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

serhiy-storchaka Nov 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Anyway, this would be just a warning. Not wrong code or missed optimization.

vstinner Nov 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Nov 5, 2025
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. 👍

Comment thread Modules/posixmodule.c
{
#if SIZEOF_LONG >= 8
#if SIZEOF_TIME_T == 4
return PyLong_FromLongLong(sec * SEC_TO_NS + nsec);

serhiy-storchaka Nov 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Anyway, this would be just a warning. Not wrong code or missed optimization.

vstinner commented Nov 5, 2025

Copy link
Copy Markdown
Member Author

The warnings are still fixed with this PR on x86 Debian: https://buildbot.python.org/#/builders/1285/builds/531

vstinner marked this pull request as ready for review November 5, 2025 20:16
vstinner merged commit 986bb0a into python:main Nov 5, 2025
53 of 54 checks passed
vstinner deleted the statx_sec_to_ns2 branch November 5, 2025 20:16
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
pythongh-83714: Fix stat_nanosecond_timestamp() for 32-bit time_t (py…
2d1c846 
…thon#141069)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL