classification
Title: [Patch] Also stop using localtime() in timemodule
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: EdSchouten, akira, belopolsky, lemburg, python-dev, tim.peters, vstinner
Priority: normal Keywords: patch

Created on 2016-09-14 12:35 by EdSchouten, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
patch-no-gmtime-localtime EdSchouten, 2016-09-14 12:35 Patch for using gmtime_r()/localtime_r() review
patch-pytime-localtime-gmtime EdSchouten, 2016-09-14 20:27 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (14)
msg276421 - (view) Author: Ed Schouten (EdSchouten) * Date: 2016-09-14 12:35
In issue 28067, we changed _datetimemodule to stop using localtime() and gmtime(), which is nice. I actually needed such a change for CloudABI (https://mail.python.org/pipermail/python-dev/2016-July/145708.html) which does not provide the thread-unsafe variants. Only localtime_r() and gmtime_r() are provided.

If we're starting to make use of these functions, let's complete this by also changing timemodule to use them. I've attached a patch.

Maybe it now makes sense to move the Windows wrappers to some header file, so that they can be shared between both modules. What would be the right location for this?
msg276471 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 18:27
Yes, I think this is a good idea.  I was hesitant to make this change while #22798 was open because I thought we may end up exposing changes to tzname caused by localtime and friends.

I also believe we can classify this as a bug-fix because side-effects of localtime are often undesirable.  Nevertheless, I would like to hear from more people before accepting this.
msg276472 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 18:30
> What would be the right location for [the Windows wrappers]?

I would say Include/pytime.h:

/**************************************************************************
Symbols and macros to supply platform-independent interfaces to time related
functions and constants
**************************************************************************/
msg276474 - (view) Author: Ed Schouten (EdSchouten) * Date: 2016-09-14 18:41
Hi Alexander,

I'm absolutely no expert when it comes to the Python codebase, so I've got a question. If we're going to movein this to Include/pytime.h, we should likely introduce full wrappers that have a name starting with _PyTime_, right?

This header seem to be part of the installation, so we should likely not declare any commonly used symbols there.
msg276475 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 18:56
> we should likely introduce full wrappers that have a name starting with _PyTime_, right?


Yes, and I would like to give some thought to what the best API would be. The two choices are to emulate localtime_r on Windows or emulate localtime_s on POSIX. While localtime_r is probably a better known function, localtime_s has been standardized by C11 and may appear on POSIX platforms in the future.

Also, I think _PyTime_localtime_r/s should include the 

 #ifdef EINVAL
         if (errno == 0)
             errno = EINVAL;
 #endif

code that is repeated everywhere in the current codebase.
msg276479 - (view) Author: Ed Schouten (EdSchouten) * Date: 2016-09-14 19:21
As a person who keeps a close eye on the Austin Group mailing lists (i.e., 'the POSIX working group'), my guess is that it's very unlikely that POSIX will ever add those *_s() extensions. Here's a discussion on Reddit that actually captures all of the arguments pretty well:

https://www.reddit.com/r/C_Programming/comments/3ivi77/eli5_why_does_glibc_still_not_support_the/

That said, any API will do. The localtime_r() function has the disadvantage that the return value is a bit odd: can it return any other tm structure than the one provided? localtime_s() is a bit weird in that its input argument is stored after the output argument. Both functions also unnecessarily pass the time_t by reference. Maybe we can just pick a prototype that's as Pythonesque as possible that also fixes these shortcomings. Any thoughts?
msg276480 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 19:25
> Maybe we can just pick a prototype that's as Pythonesque as possible that also fixes these shortcomings. Any thoughts?

Yes, and just call it _PyTime_localtime without the ugly suffix.
msg276484 - (view) Author: Ed Schouten (EdSchouten) * Date: 2016-09-14 20:27
Does this patch look all right to you?
msg276487 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 21:06
Very nice.  I'll give it a week or two for the others to chime in and commit if I don't hear any objections.
msg276488 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 21:12
I see that you picked localtime_s-like order of arguments.  While I have no personal preference, I wonder why you prefer output to follow input.  The usual UNIX/C convention is the opposite.  Interfaces like sprintf, strcat, strftime and many other have output first.  I think the logic is that this is the order in variable assignment OUTPUT = INPUT.
msg276489 - (view) Author: Ed Schouten (EdSchouten) * Date: 2016-09-14 21:18
I've been brainwashed by https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering over the last couple of years, which is why I thought `localtime()/localtime_r()`'s way of ordering the arguments made most sense here. ;-)
msg276490 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-14 21:29
> I thought `[...]localtime_r()`'s way of ordering the arguments made most sense here.

Right. I keep forgetting which one is localtime_r and which is localtime_s. I don't think there is any preference in the Python codebase. (PEP 8 is silent on this point.)

Use of time_t instead of time_t* makes it obvious which argument is input, so as I said, for me the order does not matter.  Google style guide is reason enough to pick the order.
msg277663 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-28 21:32
New changeset c81b9107ec42 by Alexander Belopolsky in branch '3.6':
Issue #28148: Stop using localtime() and gmtime() in the time module.
https://hg.python.org/cpython/rev/c81b9107ec42
msg277665 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-28 21:40
New changeset 3afad465b3e1 by Alexander Belopolsky in branch '3.6':
Issue #28148: Added a NEWS entry.
https://hg.python.org/cpython/rev/3afad465b3e1
History
Date User Action Args
2017-03-31 16:36:25dstufftsetpull_requests: + pull_request993
2016-09-28 21:41:54belopolskysetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2016-09-28 21:40:35python-devsetmessages: + msg277665
2016-09-28 21:32:41python-devsetnosy: + python-dev
messages: + msg277663
2016-09-14 21:30:53pitrousetnosy: - pitrou
2016-09-14 21:29:28belopolskysetmessages: + msg276490
2016-09-14 21:18:02EdSchoutensetmessages: + msg276489
2016-09-14 21:12:49belopolskysetmessages: + msg276488
2016-09-14 21:06:11belopolskysetkeywords: + patch

stage: patch review -> commit review
messages: + msg276487
versions: + Python 3.7
2016-09-14 20:27:57EdSchoutensetfiles: + patch-pytime-localtime-gmtime

messages: + msg276484
2016-09-14 19:25:01belopolskysetmessages: + msg276480
2016-09-14 19:21:42EdSchoutensetmessages: + msg276479
2016-09-14 18:56:34belopolskysetmessages: + msg276475
2016-09-14 18:41:57EdSchoutensetmessages: + msg276474
2016-09-14 18:30:34belopolskysetmessages: + msg276472
2016-09-14 18:27:13belopolskysetnosy: + lemburg, tim.peters, belopolsky, pitrou, vstinner, akira
messages: + msg276471

assignee: belopolsky
type: behavior
stage: patch review
2016-09-14 16:45:15EdSchoutensettitle: Also stop using localtime() in timemodule -> [Patch] Also stop using localtime() in timemodule
2016-09-14 12:35:29EdSchoutencreate