-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-80620: Support negative values in fromtimestamp
on Windows using 0 + timedelta
#134461
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
base: main
Are you sure you want to change the base?
Conversation
…te negative timestamps.
Misc/NEWS.d/next/Windows/2025-05-21-12-29-59.gh-issue-80620.WKel4J.rst
Outdated
Show resolved
Hide resolved
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.
It's almost good, I just have a few last nitpicks :)
@@ -3241,6 +3241,13 @@ date_new(PyTypeObject *type, PyObject *args, PyObject *kw) | |||
return self; | |||
} | |||
|
|||
static PyObject *add_datetime_timedelta(PyDateTime_DateTime *date, | |||
PyDateTime_Delta *delta, | |||
int factor); |
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.
Can you remove the duplicated definition line 3762?
Can you also move these two definitions at line 154? In the "Forward declarations" block.
if (_PyTime_localtime(0, &tm) != 0) | ||
return NULL; |
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 (_PyTime_localtime(0, &tm) != 0) | |
return NULL; | |
if (_PyTime_localtime(0, &tm) != 0) { | |
return NULL; | |
} |
if (delta == NULL) { | ||
Py_DECREF(date); | ||
return NULL; | ||
} |
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.
You can move negate and result definitions here. There is no need to initialize result to NULL.
if (delta == NULL) { | ||
Py_DECREF(dt); | ||
return NULL; | ||
} |
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.
You can move result definition here. There is no need to initialize it to NULL.
Thanks for this PR! I think that it is a net benefit over what we have today, but I do think it's probably pretty inefficient, and considering we're already in C-land, maybe we can just directly call the C functions underlying timedelta addition instead of constructing a timedelta for this? I think what you need to do to accomplish that is basically convert the int year = 1970;
int month = 1;
int day = 1;
int hour = 0;
int minute = 0;
int second = seconds;
int microsecond = microseconds;
normalize_datetime(&year, &month, &day, &hour, &minute, &second, µsecond); Then construct a |
Calling |
I am suggesting that we make the proper conversions, which will be almost certainly be dramatically more efficient than converting everything to In the end it's not a big deal to be inefficient here, because it's a pretty rare situation that wasn't even possible before, but we might as well do it now rather than wait for someone to notice and show up with a new issue "negative timestamps on windows are dramatically slower than positive ones!" |
Actually it occurs to me that if we just split into seconds and microseconds we will overflow in the late 19th century. I think it is best to convert to (hours, seconds, microseconds), then normalize the datetime. That will give 244k years, which is well outside the range supported by datetime. |
Support negative values in
datetime.date.fromtimestamp
anddatetime.datetime.fromtimestamp
on Windows, by substituting 0 and usingdatetime.timedelta
to go back in time.