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?

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.

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

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.

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

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?

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

Does this patch look all right to you?

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.

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.

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. ;-)

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

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

New changeset 3afad465b3e1 by Alexander Belopolsky in branch '3.6':
Issue #28148: Added a NEWS entry.
https://hg.python.org/cpython/rev/3afad465b3e1