-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-34373: fix test_mktime and test_pthread_getcpuclickid tests on AIX #8726
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
Conversation
add range checking for _PyTime_localtime (for AIX)
Modules/timemodule.c
Outdated
clk = mktime(&buf); | ||
buf.tm_year = year; | ||
|
||
if (buf.tm_wday != -1 && days) |
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.
Please, use braces here according to PEP 7.
Modules/timemodule.c
Outdated
int days = 0; | ||
|
||
/* year < 1970 */ | ||
if (year < 70) do |
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.
Please, put the braces in the same line as the "if" (https://www.python.org/dev/peps/pep-0007/#id5)
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.
Actually, I was wondering if this might be useful for other platforms as well (that cannot handle negative times) - so I put it in additional brackets. I'll use your recommendation and make it "AIX only".
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.
@aixtools Mi comment only refers to a style issue (the braces should open on the same line and not in a new line:
if (year < 70) do {
...
}
as opposed to
if (year < 70) do
{
...
}
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.
This mostly looks good to me, just one question regarding whether or not the ctypes
dependency in the test case can be avoided (hence the approving review, rather than an immediate merge).
tt is now modified before it is checked if it's equal to (time_t)-1. It looks like a regression to me: I wrote PR #12726 to propose a fix. |
fix test_mktime and test_pthread_getcpuclickid tests
add range checking for _PyTime_localtime (for AIX)
https://bugs.python.org/issue34373