FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
time: add Round and Truncate

New in Go 1 will be nanosecond precision in the result of time.Now on
Linux.
This will break code that stores time in external formats at microsecond
precision, reads it back, and expects to get exactly the same time.

Code like that can be fixed by using time.Now().Round(time.Microsecond)
instead of time.Now() in those contexts.

Please review this at https://codereview.appspot.com/6903050/

Affected files:
M doc/go1.1.html
M src/pkg/time/example_test.go
M src/pkg/time/time.go
M src/pkg/time/time_test.go

Search Discussions

  • Brad Fitzpatrick at Dec 7, 2012 at 10:42 pm
    s/precision/resolution/g?
    On Fri, Dec 7, 2012 at 10:35 AM, wrote:

    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello golang-dev@googlegroups.com,

    I'd like you to review this change to
    https://go.googlecode.com/hg/


    Description:
    time: add Round and Truncate

    New in Go 1 will be nanosecond precision in the result of time.Now on
    Linux.
    This will break code that stores time in external formats at microsecond
    precision, reads it back, and expects to get exactly the same time.

    Code like that can be fixed by using time.Now().Round(time.**Microsecond)
    instead of time.Now() in those contexts.

    Please review this at https://codereview.appspot.**com/6903050/<https://codereview.appspot.com/6903050/>

    Affected files:
    M doc/go1.1.html
    M src/pkg/time/example_test.go
    M src/pkg/time/time.go
    M src/pkg/time/time_test.go

  • Russ Cox at Dec 7, 2012 at 10:51 pm
    Rob signed off on the API but I'd like someone to review the code,
    especially the math. Maybe Rémy or Robert?

    Thanks.
    Russ
  • Iant at Dec 8, 2012 at 8:35 am
  • Remyoudompheng at Dec 8, 2012 at 9:51 am
    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go
    File src/pkg/time/time.go (right):

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1037
    src/pkg/time/time.go:1037: // Truncate returns the result of rounding t
    down to a multiple of d.
    (see below: "Time t is a multiple of Duration d" doesn't mean anything
    to me).

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1065
    src/pkg/time/time.go:1065: return int(t.nsec/int32(d)) & 1,
    Duration(t.nsec % int32(d))
    this is not correct, for example if d == Second. In this case I would
    expect to see t.sec&1 affect qmod2, but the code only mentions t.nsec.

    This code is correct if 2*d divides Second.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1071
    src/pkg/time/time.go:1071: return int(t.sec/d1) & 1,
    Duration(t.sec%d1)*Second + Duration(t.nsec)
    I think t.sec/d1 is not what you want in case t.sec < 0 (which is highly
    unlikely anyway). The comment for div should explain whether the
    returned r is always >= 0.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1078
    src/pkg/time/time.go:1078:
    I think rounding time for a duration which is not a divisor of
    86400*time.Second or of time.Hour doesn't make sense because the result
    will depend on which day is chosen as origin of time, so it would be
    essentially unspecified.

    But rounding to 360ms (which is time.Hour / 10000) can make sense, and
    is not is the previous easy cases.

    Rounding a time.Time to 7 seconds (t.Truncate(7*time.Second)) and a Unix
    time to 7 seconds (for example, u := t.Unix(); t = time.Unix(u-u%7, 0))
    will give different results even for reasonable values of t, and this
    looks very confusing.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1080
    src/pkg/time/time.go:1080: tmp := (uint64(t.sec) >> 32) * 1e9
    I'm not sure this makes any sense if t.sec < 0.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1082
    src/pkg/time/time.go:1082: u0 := tmp << 32
    if the goal is u1<<64|u0 = (tmp&^0xffffffff)*1e9 here, I think u0 should
    be (tmp&0xffffffff) << 32.

    I'm not yet in shape to look at the things below.

    https://codereview.appspot.com/6903050/
  • Remyoudompheng at Dec 8, 2012 at 10:01 am
    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/example_test.go
    File src/pkg/time/example_test.go (right):

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/example_test.go#newcode60
    src/pkg/time/example_test.go:60: func ExampleTime_Round() {
    there should be a test demonstrating the round-to-even behaviour in very
    simple cases (which I think are broken, see previuos comment).

    Parse("2012 Dec 07 12:15:30.5").Round(time.Second) -> 12:15:30
    Parse("2012 Dec 07 12:15:31.5").Round(time.Second) -> 12:15:32

    https://codereview.appspot.com/6903050/
  • Remyoudompheng at Dec 8, 2012 at 11:01 pm
    Another example of how "round-even" may have an unclear meaning. Suppose
    I am calling t.Round(time.Hour). I expect to get an even number of hours
    if I'm halfway. But:

    - if t is 16:30:00 UTC, it gives 16:00:00 UTC
    - if t is 16:30:00 CET (UTC+1), it gives 17:00:00 CET ?
    - if t is 16:30:00 CEST (UTC+2), it gives 16:00:00 CEST ?

    Is it something a sane user can expect?

    Rémy.

    https://codereview.appspot.com/6903050/
  • Rsc at Dec 9, 2012 at 6:58 am

    On 2012/12/08 23:01:16, remyoudompheng wrote:
    Another example of how "round-even" may have an unclear meaning.
    Suppose I am
    calling t.Round(time.Hour). I expect to get an even number of hours if I'm
    halfway. But:
    - if t is 16:30:00 UTC, it gives 16:00:00 UTC
    - if t is 16:30:00 CET (UTC+1), it gives 17:00:00 CET ?
    - if t is 16:30:00 CEST (UTC+2), it gives 16:00:00 CEST ?
    Is it something a sane user can expect?
    Yeah, that's annoying. It's probably better to round up always, which
    I've done now.
    Note that if you are in a fractional time zone that is n hours + 30
    minutes off from UTC,
    rounding to hour produces times ending in :30 in the local zone.
    Similarly, in any non-UTC local zone, rounding to 1*Day does not produce
    00:00:00.

    These are all implications of using the zero time instant as zero.
    I think it is better to live with them than to make the computation
    location-dependent.

    https://codereview.appspot.com/6903050/
  • Rsc at Dec 9, 2012 at 6:59 am
    @bradfitz: we use precision elsewhere in the package instead of
    resolution



    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go
    File src/pkg/time/time.go (right):

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1037
    src/pkg/time/time.go:1037: // Truncate returns the result of rounding t
    down to a multiple of d.
    On 2012/12/08 09:51:50, remyoudompheng wrote:
    (see below: "Time t is a multiple of Duration d" doesn't mean anything
    to me).

    I understand what you're saying, but I don't have a good way to
    describe it that doesn't cause more confusion than it resolves.
    For cases people will use it for - like Truncate(time.Millisecond),
    the choice of zero is irrelevant, and I want to avoid making people
    think zero matters more than it does.

    Changed to:

    // Truncate returns the result of rounding t down to a multiple of d
    (since the zero time).

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1065
    src/pkg/time/time.go:1065: return int(t.nsec/int32(d)) & 1,
    Duration(t.nsec % int32(d))
    On 2012/12/08 09:51:50, remyoudompheng wrote:
    this is not correct, for example if d == Second. In this case I would expect to
    see t.sec&1 affect qmod2, but the code only mentions t.nsec.
    This code is correct if 2*d divides Second.
    Nice catch, thanks. Made that the condition.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1071
    src/pkg/time/time.go:1071: return int(t.sec/d1) & 1,
    Duration(t.sec%d1)*Second + Duration(t.nsec)
    On 2012/12/08 09:51:50, remyoudompheng wrote:
    I think t.sec/d1 is not what you want in case t.sec < 0 (which is highly
    unlikely anyway). The comment for div should explain whether the
    returned r is
    always >= 0.
    You're right. I forgot we allowed t.sec >= 0. Fixed this whole routine
    to handle that.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1078
    src/pkg/time/time.go:1078:
    On 2012/12/08 09:51:50, remyoudompheng wrote:
    I think rounding time for a duration which is not a divisor of
    86400*time.Second
    or of time.Hour doesn't make sense because the result will depend on which day
    is chosen as origin of time, so it would be essentially unspecified.
    Agreed until the last word. We've defined where 0 (the zero Time) is, so
    it's all specified. I agree it's a useless thing to do, but it's all
    specified, and if asked to round to 3 nanoseconds or 3 days, I'd rather
    give a consistent result than panic.
    Rounding a time.Time to 7 seconds (t.Truncate(7*time.Second)) and a
    Unix time to
    7 seconds (for example, u := t.Unix(); t = time.Unix(u-u%7, 0)) will give
    different results even for reasonable values of t, and this looks very
    confusing.
    It depends. If you actually notice that it's not where you think it is,
    you're looking _very_ close and will probably figure out where 0 is. On
    the other hand, if you just want to do something every 7 seconds and
    want a consistent way to generate times 7 seconds apart, this works
    fine.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1080
    src/pkg/time/time.go:1080: tmp := (uint64(t.sec) >> 32) * 1e9
    On 2012/12/08 09:51:50, remyoudompheng wrote:
    I'm not sure this makes any sense if t.sec < 0.
    Fixed.

    https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1082
    src/pkg/time/time.go:1082: u0 := tmp << 32
    On 2012/12/08 09:51:50, remyoudompheng wrote:
    if the goal is u1<<64|u0 = (tmp&^0xffffffff)*1e9 here, I think u0 should be
    (tmp&0xffffffff) << 32.
    tmp<<32 and (tmp&0xffffffff)<<32 are the same thing, no?
    I'm not yet in shape to look at the things below.
    No worries. Thanks. The test against math/big makes me pretty confident.

    https://codereview.appspot.com/6903050/
  • Russ Cox at Dec 9, 2012 at 7:00 am
    PTAL rémy
  • Remyoudompheng at Dec 9, 2012 at 8:24 am
    LGTM after the test fix


    https://codereview.appspot.com/6903050/diff/4/src/pkg/time/time_test.go
    File src/pkg/time/time_test.go (right):

    https://codereview.appspot.com/6903050/diff/4/src/pkg/time/time_test.go#newcode340
    src/pkg/time/time_test.go:340: for Second%d == 0 {
    i think your condition is reversed. You want to stop when Second%d == 0.
    But if d was 500000001 you are really, really screwed, and even more on
    ARM. I suggest:

    // divisors of Second
    f1 := func(ti int64, tns int32, logdi uint32) bool {
    d := Duration(1)
    a, b := logdi%9, (logdi>>16)%9
    d <<= a
    for i := 0; i < int(b); i++ {
    d *= 5
    }
    return testOne(ti, int64(tns), int64(d))
    }
    quick.Check(f1, cfg)

    https://codereview.appspot.com/6903050/
  • Rsc at Dec 9, 2012 at 8:59 am
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=ea15e7ed6d72 ***

    time: add Round and Truncate

    New in Go 1 will be nanosecond precision in the result of time.Now on
    Linux.
    This will break code that stores time in external formats at microsecond
    precision, reads it back, and expects to get exactly the same time.

    Code like that can be fixed by using time.Now().Round(time.Microsecond)
    instead of time.Now() in those contexts.

    R=golang-dev, bradfitz, iant, remyoudompheng
    CC=golang-dev
    https://codereview.appspot.com/6903050


    https://codereview.appspot.com/6903050/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 7, '12 at 6:35p
activeDec 9, '12 at 8:59a
posts12
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase