houseabsolute / Time-Local

Efficiently compute time from local and GMT time
https://metacpan.org/release/Time-Local/
Other
4 stars 11 forks source link

timegm() returns float when seconds are in float format #18

Closed justnoxx closed 1 year ago

justnoxx commented 3 years ago

Hello!

We are upgrading our old system from perl-5.8.9 to latest perl and found that timegm() has different behaviour now.

Previously we had the Time::Local version 1.19, now we have the Time::Local version 1.30.

The code is:

#!/usr/bin/perl

use strict;
use warnings;
use Time::Local;

print "Time::Local Version: $Time::Local::VERSION\n";
print timegm(33.354, 01, 05,05,10-1,2008), "\n";
print timegm(33.999, 01, 05,05,10-1,2008), "\n";

The result for 5.8.9:

Time::Local Version: 1.1901
1223182893
1223182893

For 5.10.1:

Time::Local Version: 1.1901
1223182893
1223182893

For 5.12.5 (as you can see that 1.1901_01 is the first affected version):

Time::Local Version: 1.1901_01
1223182893.354
1223182893.999

And for 5.34:

Time::Local Version: 1.30
1223182893.354
1223182893.999

So the main question is what is the right behaviour and if it is not a bug could you please add the information about return format in the next release?

Thanks.

/Dmitriy

autarch commented 3 years ago

Digging through history I see that this changed in Perl 5.12 as part of making the Perl core always use a 64-bit time_t. Given that no one has complained since then and it's been > 10 years, I think the new behavior is fine, and it's certainly more correct and less surprising.

I'm happy to accept a doc patch for this, but it seems to me that since this isn't actually doing anything surprising (you get back what you put in), I'm not sure what the docs should say.

n1vux commented 3 years ago

Given doc says

That means that calling timelocal_posix( localtime($value) ) will always give you the same $value you started with. one could argue that it's already implicitly documented, but

I would suggest the section Limits of time_t is an appropriate spot in Doc to announce this ... since the return is very much departing from time_t in this case, but this section seemingly implies time_t is a relevant limit.

(I'd also like an explicit statement whether the *_modern variants have any equivalent identity relation.)

autarch commented 3 years ago

I was digging into this some more today to add some tests, but it turns out the behavior of timelocal when given a non-integer seconds value is bizarre:

$>  perl -MTime::Local=timelocal_modern -E 'say timelocal_modern( 33.354, 1, 5, 5, 7, 2008 )'
1217930494.062

This happens because inside timelocal we do some math that involves calling _timegm(localtime(...)). And the localtime call strips off the non-integer portion of the value.

So now I'm wondering if the best thing to do is to just add back the use integer call to prevent this mess, or maybe to validate that seconds is an integer (except for *_nocheck subs).

autarch commented 2 years ago

I made a trial release restoring the use integer behavior - https://metacpan.org/release/DROLSKY/Time-Local-1.31-TRIAL

craigberry commented 2 years ago

As far as I can find, the C standard does not specify a type for time_t, and POSIX just says it's an arithmetic type of appropriate size. So I'm not sure assuming or requiring to be an integral type will always work.

scottchiefbaker commented 1 year ago

I was accidentally using fractional seconds with timegm_modern() and it was working fine. When I put my module on CPAN the testers kicked in and now I'm getting a bunch of errors using fractional times on mswin32:

http://matrix.cpantesters.org/?dist=Date-Parse-Modern%200.3_02

It appears that fractional seconds work fine with a newish Time::Local but not on Windows. I put a work around in my module for now.

autarch commented 1 year ago

Hmm, I could add a similar workaround in Time::Local to handle non-integer second values. That might be better than just forcing everything to be an integer internally.

scottchiefbaker commented 1 year ago

@autarch that would be my recommendation... it's pretty simple.

autarch commented 1 year ago

If only it were that easy. When using negative epoch values all sorts of fun floating point rounding errors start creeping in. I'm not sure if there's a good way to prevent this.

autarch commented 1 year ago

I just released a trial release, 1.32, which does this sub-second handling.

autarch commented 1 year ago

I just released a stable release with this fix, v1.35.