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

Define _PyTime_MIN/MAX with macros specific to the actual type of _PyTime_t by sir-sigurd · Pull Request #15384 · python/cpython · GitHub

/ cpython Public

Define _PyTime_MIN/MAX with macros specific to the actual type of _PyTime_t - #15384

Merged
vstinner merged 1 commit into
python:masterfrom
sir-sigurd:py-time-macro
Aug 23, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Define _PyTime_MIN/MAX with macros specific to the actual type of _PyTime_t#15384
vstinner merged 1 commit into
python:masterfrom
sir-sigurd:py-time-macro

Conversation

sir-sigurd commented Aug 22, 2019

Copy link
Copy Markdown
Contributor

No description provided.

sir-sigurd mentioned this pull request Aug 22, 2019
Merged
aeros reviewed Aug 23, 2019
View reviewed changes

aeros left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Thanks for the PR @sir-sigurd.

This looks like a minor enough adjustment to warrant a skip issue and skip news, particularly since it's modifying internal constants. I'll notify the datetime experts (although this might not be needed, since they were automatically added as reviewers).

/cc @abalkin @pganssle

aeros added skip issue skip news type-feature A feature request or enhancement labels Aug 23, 2019

pganssle commented Aug 23, 2019

Copy link
Copy Markdown
Member

It's not clear to me:

  1. What problem this is solving.
  2. Why PY_LLONG_MIN and PY_LLONG_MAX were chosen in the first place.

The principle of Chesterton's Fence applies here, so I think we need to figure out whether this was actually a mistake or a deliberate choice. Looks like @vstinner added these macros in this commit. Maybe he can comment as to why he chose PY_LLONG_MIN and PY_LLONG_MAX over INT64_MIN and INT64_MAX here.

vstinner commented Aug 23, 2019

Copy link
Copy Markdown
Member

The principle of Chesterton's Fence applies here, so I think we need to figure out whether this was actually a mistake or a deliberate choice. Looks like @vstinner added these macros in this commit. Maybe he can comment as to why he chose PY_LLONG_MIN and PY_LLONG_MAX over INT64_MIN and INT64_MAX here.

There is no secret: it's a bug :-)

I don't recall the detail, but I recall that I wasn't sure about if using int64_t would cause any portability issue. There were very old discussions about using <stdint.h> and why PY_LONG_LONG was needed, etc.

I chose to attempt to use int64_t and see who is going to complain. Years later: nobody complains, so it seems like the (Python) World is ready for int64_t :-D

Maybe the first iteration of my change used "typedef long _PyTime_t;" and then I changed my mind.

vstinner merged commit 8e76c45 into python:master Aug 23, 2019

miss-islington commented Aug 23, 2019

Copy link
Copy Markdown
Contributor

Thanks @sir-sigurd for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

miss-islington commented Aug 23, 2019

Copy link
Copy Markdown
Contributor

Sorry @sir-sigurd and @vstinner, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 8e76c456226438f2e4931ce7baf05ac8faae34a1 3.8

bedevere-bot commented Aug 23, 2019

Copy link
Copy Markdown

GH-15425 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 23, 2019
Fix _PyTime_MIN/MAX values (pythonGH-15384)
c86d34f 
_PyTime_t type is defined as int64_t, and so min/max are INT64_MIN/INT64_MAX,
not PY_LLONG_MIN/PY_LLONG_MAX.
(cherry picked from commit 8e76c45)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>

vstinner commented Aug 23, 2019

Copy link
Copy Markdown
Member

If you are curious about _PyTime API, I wrote two articles about its history :-)

sir-sigurd deleted the py-time-macro branch August 23, 2019 15:40

bedevere-bot commented Aug 23, 2019

Copy link
Copy Markdown

GH-15426 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Aug 23, 2019
Fix _PyTime_MIN/MAX values (GH-15384)
8fac472 
_PyTime_t type is defined as int64_t, and so min/max are INT64_MIN/INT64_MAX,
not PY_LLONG_MIN/PY_LLONG_MAX.
(cherry picked from commit 8e76c45)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
vstinner added a commit that referenced this pull request Aug 23, 2019
Fix _PyTime_MIN/MAX values (GH-15384) (GH-15426)
944aa01 
_PyTime_t type is defined as int64_t, and so min/max are INT64_MIN/INT64_MAX,
not PY_LLONG_MIN/PY_LLONG_MAX.

(cherry picked from commit 8e76c45)

aeros commented Aug 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@vstinner Thanks for the detailed explanation and context!

There were very old discussions about using <stdint.h> and why PY_LONG_LONG was needed, etc.

Are there still any use cases for Py_LONG_LONG types at this point (within the Python C-API)?

vstinner commented Aug 26, 2019

Copy link
Copy Markdown
Member

Are there still any use cases for Py_LONG_LONG types at this point (within the Python C-API)?

long long is now always avaialble and PY_LONG_LONG is always defined as "long long". The macro is no more used in the Python code base, except in dict_get_version() of _testcapimodule.c. Maybe this function can be updated.

aeros commented Aug 27, 2019

Copy link
Copy Markdown
Contributor

long long is now always avaialble and PY_LONG_LONG is always defined as "long long". The macro is no more used in the Python code base, except in dict_get_version() of _testcapimodule.c. Maybe this function can be updated.

@vstinner I've been looking to get more experience with C-API development, perhaps this would serve as a decent introductory project for me to work on.

sir-sigurd commented Aug 27, 2019

Copy link
Copy Markdown
Contributor Author

@aeros167 there is already #15386.

aeros commented Aug 27, 2019

Copy link
Copy Markdown
Contributor

@aeros167 there is already #15386.

Ah okay, thanks for letting me know.

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
Fix _PyTime_MIN/MAX values (pythonGH-15384)
ddd39c0 
_PyTime_t type is defined as int64_t, and so min/max are INT64_MIN/INT64_MAX,
not PY_LLONG_MIN/PY_LLONG_MAX.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Fix _PyTime_MIN/MAX values (pythonGH-15384)
b1ccf5c 
_PyTime_t type is defined as int64_t, and so min/max are INT64_MIN/INT64_MAX,
not PY_LLONG_MIN/PY_LLONG_MAX.
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
Fix _PyTime_MIN/MAX values (pythonGH-15384)
6452487 
_PyTime_t type is defined as int64_t, and so min/max are INT64_MIN/INT64_MAX,
not PY_LLONG_MIN/PY_LLONG_MAX.
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

abalkin Awaiting requested review from abalkin
pganssle Awaiting requested review from pganssle
1 more reviewer
aeros aeros left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

vstinner

Labels

skip issue skip news type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL