Skip to content

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

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

Copy link
Copy Markdown
Contributor

No description provided.

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

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

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

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

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

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

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

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

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 sir-sigurd deleted the py-time-macro branch August 23, 2019 15:40
@bedevere-bot

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

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

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

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

sir-sigurd commented Aug 27, 2019

Copy link
Copy Markdown
Contributor Author

@aeros167 there is already #15386.

@aeros

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

@abalkin abalkin Awaiting requested review from abalkin

@pganssle pganssle Awaiting requested review from pganssle

1 more reviewer

@aeros aeros aeros left review comments

Reviewers whose approvals may not affect merge requirements

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.

7 participants

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