Skip to content
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

Jump to bottom

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

Merged
merged 1 commit into from Aug 23, 2019
Merged

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

merged 1 commit into from Aug 23, 2019

Conversation

sir-sigurd
Copy link
Contributor

@sir-sigurd sir-sigurd commented Aug 22, 2019

No description provided.

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

@aeros aeros left a comment •
edited

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 aeros added skip issue skip news type-feature A feature request or enhancement labels Aug 23, 2019
@pganssle
Copy link
Member

pganssle commented Aug 23, 2019

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
Copy link
Member

vstinner commented Aug 23, 2019

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 vstinner merged commit 8e76c45 into python:master Aug 23, 2019
@miss-islington
Copy link
Contributor

miss-islington commented Aug 23, 2019

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
Copy link
Contributor

miss-islington commented Aug 23, 2019

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
Copy link

bedevere-bot commented Aug 23, 2019

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
@sir-sigurd @miss-islington
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
Copy link
Member

vstinner commented Aug 23, 2019

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

@sir-sigurd sir-sigurd deleted the py-time-macro branch Aug 23, 2019
@bedevere-bot
Copy link

bedevere-bot commented Aug 23, 2019

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
@miss-islington @sir-sigurd
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
@vstinner
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
Copy link
Member

aeros commented Aug 23, 2019 •
edited

@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
Copy link
Member

vstinner commented Aug 26, 2019

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
Copy link
Member

aeros commented Aug 27, 2019

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
Copy link
Contributor Author

sir-sigurd commented Aug 27, 2019

@aeros167 there is already #15386.

@aeros
Copy link
Member

aeros commented Aug 27, 2019

@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
@sir-sigurd @lisroach
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
@sir-sigurd @DinoV
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
@sir-sigurd @websurfer5
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros

@abalkin abalkin

@pganssle pganssle

Assignees

@vstinner 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.

None yet
7 participants
@sir-sigurd @pganssle @vstinner @miss-islington @bedevere-bot @aeros @the-knights-who-say-ni