[issue28148] [Patch] Also stop using localtime() in timemodule

[Donald Stufft]

Changes by Donald Stufft :


--
pull_requests: +993

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Changes by Alexander Belopolsky :


--
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
nosy:  -pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Ed Schouten]

Ed Schouten added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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.

--
keywords: +patch
stage: patch review -> commit review
versions: +Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Ed Schouten]

Ed Schouten added the comment:

Does this patch look all right to you?

--
Added file: http://bugs.python.org/file44667/patch-pytime-localtime-gmtime

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Ed Schouten]

Ed Schouten added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Ed Schouten]

Ed Schouten added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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.

--
assignee:  -> belopolsky
nosy: +akira, belopolsky, haypo, lemburg, pitrou, tim.peters
stage:  -> patch review
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28148] [Patch] Also stop using localtime() in timemodule

[Ed Schouten]

Changes by Ed Schouten :


--
title: Also stop using localtime() in timemodule -> [Patch] Also stop using 
localtime() in timemodule

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com