-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-31339: Rewrite time.asctime() and time.ctime() #3293
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
Backport and adapt the _asctime() function from the master branch to not depend on the implementation of asctime() and ctime() from the external C library. This change fixes a bug when Python is run using the musl C library.
unicode = PyUnicode_FromFormat( | ||
"%s %s%3d %.2d:%.2d:%.2d %d", | ||
wday_name[timeptr->tm_wday], | ||
mon_name[timeptr->tm_mon], |
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.
The function should verify that tm_wday
and tm_mon
are not under- or overflowing.
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.
Such check is done in the caller:
>>> time.asctime((2017, 13, 4, 22, 44, 18, 0, 247, 1))
ValueError: month out of range
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.
Ooops, sorry, I was checking Python 3.
You are right that my PR is wrong:
>>> time.asctime((2017, 13, 4, 22, 44, 18, 0, 247, 1))
'Mon ??? 4 22:44:18 2017'
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 now fixed:
>>> time.asctime((2017, 13, 4, 22, 44, 18, 0, 247, 1))
ValueError: month out of range
Ok, I also backported checktm() function from master and modified time.asctime() and time.strftime() to use it (as done in master). So my PR now changes time.asctime(), time.ctime(), time.strftime(). |
PyErr_SetString(PyExc_ValueError, "minute out of range"); | ||
return 0; | ||
} | ||
if (buf->tm_sec < 0 || buf->tm_sec > 61) { |
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.
Are you sure that leap seconds are limited to maximum 2 seconds?
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.
IMHO supporting tm_sec=61 is already crazy. I expect seconds to be in the inclusive range 0..60. Linux asctime() and mktime() documentation say:
int tm_sec; /* Seconds (0-60) */
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.
I expect a minimum of one leap second per day. Currently, it seems to be limited to 1 leap second per UTC month. In practice, it's one leap second every 18 months.
https://en.wikipedia.org/wiki/Leap_second#Insertion_of_leap_seconds
Note: I copied the code from master without reading it :-)
Would you mind to review this change @tiran? (Approve it if it looks good to you.) |
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.
LGTM
I'm new at commenting on this project so apologies if this is not the appropriate place to do so. From what I can see (upgrading from python 2.7.13->2.7.15), the string format on line 648 of Modules/timemodule.c causes a different output from time.ctime() and time.asctime(t) for "days of the month < 10"
The "%3d" in this change removes the leading zero on the tm_mday but maintains a leading space. Just looking for feedback on what the intention was and if this is a bug. python 2.7.13:
python 2.7.15:
Note, the string with this change includes two spaces between "Dec" and "6" which also looks awkward. Original Post: |
I created https://bugs.python.org/issue35469 for you ;-) |
Going to say it here too - the old behaviour was to rely on whatever the C |
Backport and adapt the _asctime() function from the master branch to
not depend on the implementation of asctime() and ctime() from the
external C library. This change fixes a bug when Python is run using
the musl C library.
https://bugs.python.org/issue31339