code review 5392041: time: new Time, Duration, ZoneInfo types

time: new Time, Duration, ZoneInfo types

[rsc, 2011-11-17 22:34:15]

Hello r (cc: golang-dev@googlegroups.com),

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

[bradfitz, 2011-11-17 22:50:27]

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode97
src/pkg/time/time.go:97: // String returns the English name of the month
("January", "Feburary", ...).
February

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode370
src/pkg/time/time.go:370: // in the range [0, 999999999].  It is equivalent to
int(t.Nanoseconds)
t.Nanoseconds isn't exported.  not worth documenting publicly?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode390
src/pkg/time/time.go:390: // Duration returns a string representing the duration
in the form "72h3m0.005s".
80

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode640
src/pkg/time/time.go:640: var daysBefore = [...]int32{
pretty

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode745
src/pkg/time/time.go:745: panic("time.Date: invalid day")
surprising use of panic.

could we do something like template.Must instead, wrapping an error-returning
version?

[gri, 2011-11-17 23:43:13]

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:827: }
why not just

for nbytes < 10 {
  ns *= 10
  nbytes++
}

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode201
src/pkg/time/time.go:201: // in the year -292277022399 incorrectly.
On the other hand, printing a few times in the year ... incorrectly is unlikely
to cause any problems.
?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode217
src/pkg/time/time.go:217: // No longer changes.
2nd line comment by itself is not helpful

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode523
src/pkg/time/time.go:523: // units do not have fixed lengths.
I would remove them. Minutes and hours are trivially computed if needed.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode745
src/pkg/time/time.go:745: panic("time.Date: invalid day")
fine use of panic in my mind

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
I like Location.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:23: cacheStart int64
the meaning of these fields is not obvious

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:120: tx := z.tx
no sort.Search for speed reasons?

[dsymonds, 2011-11-18 00:00:33]

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode42
src/pkg/time/time.go:42: // A nil Zone is interpreted to mean the system's local
time zone.
why local time and not UTC?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode56
src/pkg/time/time.go:56: // Equal reports whether t and u represent the same
time instant.
Did you want to note that this is different to comparing t and u with ==?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode278
src/pkg/time/time.go:278: // Weekday returns the day of the week specified by t.
This code is incorrect for dates before 1752, and again before 1582. Perhaps
call out that this package only operates on post-1752 dates?

[rsc, 2011-11-18 00:50:46]

On Thu, Nov 17, 2011 at 19:00,  <dsymonds@golang.org> wrote:
> This code is incorrect for dates before 1752, and again before 1582.
> Perhaps call out that this package only operates on post-1752 dates?

The code is correct.  It only operates on the Gregorian calendar.
I will add a note that all the calendar arithmetic is Gregorian only.

Also, no matter what your definition of broken is, the code can't be
broken in both 1752 and 1582.  Everyone only transitioned to
the Gregorian calendar once.  They just did so at different times.

Russ

[rsc, 2011-11-18 03:01:08]

On Thu, Nov 17, 2011 at 19:00,  <dsymonds@golang.org> wrote:
> Did you want to note that this is different to comparing t and u with
> ==?

Right now you cannot compare t and u with ==.

[rsc, 2011-11-18 03:02:42]

On Thu, Nov 17, 2011 at 19:00,  <dsymonds@golang.org> wrote:
> src/pkg/time/time.go:42: // A nil Zone is interpreted to mean the
> system's local time zone.
> why local time and not UTC?

Because local time is the right default.
I know I am going to get massacred for saying that,
but there's a reason date(1) doesn't print UTC unless
you ask it to, and a reason people don't set their
clocks to UTC.

There are definitely situations where you want UTC.
But not by default.

Russ

[dsymonds, 2011-11-18 03:09:28]

On Fri, Nov 18, 2011 at 2:02 PM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Nov 17, 2011 at 19:00,  <dsymonds@golang.org> wrote:
>> src/pkg/time/time.go:42: // A nil Zone is interpreted to mean the
>> system's local time zone.
>> why local time and not UTC?
>
> Because local time is the right default.
> I know I am going to get massacred for saying that,
> but there's a reason date(1) doesn't print UTC unless
> you ask it to, and a reason people don't set their
> clocks to UTC.
>
> There are definitely situations where you want UTC.
> But not by default.

But this is the internals. People are going to either call time.UTC()
or time.Now(), so there's no "default" per se.


Dave.

[rsc, 2011-11-18 03:14:01]

On Thu, Nov 17, 2011 at 22:09, David Symonds <dsymonds@golang.org> wrote:
> But this is the internals. People are going to either call time.UTC()
> or time.Now(), so there's no "default" per se.

It's true that we have control over most time.Time values.
However, the choice of interpretation for the nil does
affect how the zero Time prints.  Maybe you are right
that it should print as UTC.  That would be nice in some
ways, because it would make the zero even easier
to spot.

But it sounds like you were expecting a bigger effect
to changing the definition.  Was there something else?

Note that time.UTC is not a function: people can call
time.Now() for local time or time.Now().UTC() for UTC time.

Russ

[dsymonds, 2011-11-18 03:21:54]

On Fri, Nov 18, 2011 at 2:14 PM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Nov 17, 2011 at 22:09, David Symonds <dsymonds@golang.org> wrote:
>> But this is the internals. People are going to either call time.UTC()
>> or time.Now(), so there's no "default" per se.
>
> It's true that we have control over most time.Time values.
> However, the choice of interpretation for the nil does
> affect how the zero Time prints.  Maybe you are right
> that it should print as UTC.  That would be nice in some
> ways, because it would make the zero even easier
> to spot.
>
> But it sounds like you were expecting a bigger effect
> to changing the definition.  Was there something else?

No, it just struck me as odd that the other two fields were
UTC-relative, but then the zero value for the zone field was local
time.

Now that I write that, though, it occurs to me that using the local
time semantics for the zero value is going to make serialisation a
mess: a time.Time on my machine, serialised, sent to your machine, and
unserialised there is now a different time.Time.


Dave.

[iant, 2011-11-18 06:10:13]

FYI

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode523
src/pkg/time/time.go:523: // units do not have fixed lengths.
On 2011/11/17 23:43:13, gri wrote:
> I would remove them. Minutes and hours are trivially computed if needed.

I would keep them.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode739
src/pkg/time/time.go:739: // either of the zones.
The last clause is a bit cryptic.  I think the most important point is that the
returned time will not match what was passed in, and perhaps the comment should
just say that.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode745
src/pkg/time/time.go:745: panic("time.Date: invalid day")
(Some Chinese bloggers use dates like May 35 to avoid censorship.)  The Unix
mktime function normalizes times.  Normalizing times is useful for operations
like "same time next week".  It is not correct to simply add 7 * secondsPerDay
because that would be wrong across a zone transition.  So I think either this
function should normalize or there should be a different function which
normalizes.  Normalizing should be in the time package because it is tedious to
get right.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
On 2011/11/17 23:43:13, gri wrote:
> I like Location.

I'm not crazy about Location, because this is really a timezone, it's not a
specific location.  I think ZoneInfo is OK since the name is widely used.  I can
also think of TimeZoneOffsets or TZOffsets.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:69: name = ""
This does nothing.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:105: start = -1 << 60
Above you used << 63, here you used << 60.  Makes me wonder why they are
different.

[r, 2011-11-18 06:27:14]

http://codereview.appspot.com/5392041/diff/1056/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:22: // time.now is implemented in assembly.
is time.now even referenced here?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:288: //	Mon Jan _2 15:04:05 -0700 MST 2006
maybe quote the string?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/sys.go
File src/pkg/time/sys.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/sys.go#newcode12
src/pkg/time/sys.go:12: // readFile reads and returns the content of the named
file.
// It's a copy of io.ReadFile, here to avoid the dependency.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/tick.go
File src/pkg/time/tick.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/tick.go#newcode17
src/pkg/time/tick.go:17: // send the time, in nanoseconds, with a period of d. 
It adjusts the
"a period of d" reads oddly.
with a period specified by the duration argument.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode8
src/pkg/time/time.go:8: // A Time represents an instant in time to nanosecond
precision.
s/to/with/

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode18
src/pkg/time/time.go:18: // The zero value of type Time represents January 1,
year 1, 00:00:00 UTC.
January 1, Year 1, 00:00:00.000000000 UTC

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode31
src/pkg/time/time.go:31: // hypothetical date January 1, year 1 00:00 UTC,
ignoring leap seconds.
00:00:00 UTC is enough here.
why is it hypothetical? how about

sec gives the number of seconds elapsed since the epoch of
January 1, Year 1, 00:00:00 UTC, ignoring leap seconds.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode42
src/pkg/time/time.go:42: // A nil Zone is interpreted to mean the system's local
time zone.
On 2011/11/18 00:00:33, dsymonds wrote:
> why local time and not UTC?

agree. nil should mean UTC

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode98
src/pkg/time/time.go:98: func (m Month) String() string { return months[m] }
since you crash if m > 12, maybe you should crash if m == 0. you could do that
by zero-indexing months and subtracting 1 here.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode128
src/pkg/time/time.go:128: // The zero value for a Time is defined to be January
1, year 1, 00:00:00
i'd like to see the .000000000 here. also, i'd put the date on a line by itself
and maybe call it the zero time, as distinct from the epoch.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode135
src/pkg/time/time.go:135: // The zero value does not force a specific epoch for
the time
zero time value

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode200
src/pkg/time/time.go:200: // zones.  On the other hand, I have no problem with
printing a few times
"I" do not belong here.
By comparison, it's reasonable to mishandle some times in the year
-292277022399.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode278
src/pkg/time/time.go:278: // Weekday returns the day of the week specified by t.
On 2011/11/18 00:00:33, dsymonds wrote:
> This code is incorrect for dates before 1752, and again before 1582. Perhaps
> call out that this package only operates on post-1752 dates?

actually it depends where you live.... and the skip only happened once, wherever
you are (mostly)

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode516
src/pkg/time/time.go:516: // order to have sub-second precision.
not happy with this. i'm going to do int64(d.Seconds()) and lose data.

i would like to get integral seconds somehow without losing precision. also it
seems wrong that the duration types contradict the similarly named methods of
time, which are integral. how about making these integer and correct, and
finding a name for the others, like FloatSeconds or something? maybe there's a
better name.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode555
src/pkg/time/time.go:555: secondsPerWeek   = 7 * secondsPerDay
at least up to here, these should perhaps be exported. they show up a lot.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode640
src/pkg/time/time.go:640: var daysBefore = [...]int32{
On 2011/11/17 22:50:27, bradfitz wrote:
> pretty
yeah

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode693
src/pkg/time/time.go:693: // name of the zone (such as "EDT") and its offset in
seconds east of UTC.
it's been EST everywhere else. (but i prefer CET)

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode729
src/pkg/time/time.go:729: func isleap(year int) bool {
isLeap

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode745
src/pkg/time/time.go:745: panic("time.Date: invalid day")
On 2011/11/17 23:43:13, gri wrote:
> fine use of panic in my mind

agree, but it must be documented

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
On 2011/11/17 23:43:13, gri wrote:
> I like Location.

i do too

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:31: utcoff int    // seconds east of UTC
nice to get utc capitalized. as written it's a poorly styled name. also nice to
have the name remind us of the direction

secEastOfUTC would be a fine name.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:32: isdst  bool   // is this zone daylight savings
time?
isDST bool // is this zone Daylight Savings Time?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:35: // A zonetx represents a single time zone
transition.
zoneTrans or zoneTransition

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:37: time         int64 // transition time, in seconds
since 1970 GMT
maybe "when"? "time" is overused in this package, to put it mildly.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:38: zone         uint8 // the index of the zone that
goes into effect at that time
i'd call this index.

i'm fiddling with names, i know, but they have a big effect on the readability
of some gnarly code, dood.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:66: // given name and offset as the current zone.
explain relevance of DST

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:89: // The returned information gives the name of the
zone (such as "EST"),
how about using CET or something less parochial as your canonical example?

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:120: tx := z.tx
On 2011/11/17 23:43:13, gri wrote:
> no sort.Search for speed reasons?

avoiding dependencies

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:144: if z == nil {
i've seen this burst too many times. refactor.

z = getZoneInfo(z) or z = z.getZoneInfo()

func (z *ZoneInfo) getZoneInfo() *ZoneInfo {
if z == nil {
  z = &localZone
}
if z == &localZone {
  localOnce.Do(initLocalZone)
}
return z
}

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:160: // LookupName returns information about the time
zone with
LookupOffset

[rsc, 2011-11-18 16:25:09]

Hello r@golang.org, bradfitz@golang.org, gri@golang.org, dsymonds@golang.org,
iant@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-11-18 16:26:22]

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode8
src/pkg/time/time.go:8: // A Time represents an instant in time to nanosecond
precision.
On 2011/11/18 06:27:14, r wrote:
> s/to/with/

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode18
src/pkg/time/time.go:18: // The zero value of type Time represents January 1,
year 1, 00:00:00 UTC.
On 2011/11/18 06:27:14, r wrote:
> January 1, Year 1, 00:00:00.000000000 UTC

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode31
src/pkg/time/time.go:31: // hypothetical date January 1, year 1 00:00 UTC,
ignoring leap seconds.
On 2011/11/18 06:27:14, r wrote:
> 00:00:00 UTC is enough here.
> why is it hypothetical? how about
> 
> sec gives the number of seconds elapsed since the epoch of
> January 1, Year 1, 00:00:00 UTC, ignoring leap seconds.

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode42
src/pkg/time/time.go:42: // A nil Zone is interpreted to mean the system's local
time zone.
On 2011/11/18 06:27:14, r wrote:
> On 2011/11/18 00:00:33, dsymonds wrote:
> > why local time and not UTC?
> 
> agree. nil should mean UTC

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode56
src/pkg/time/time.go:56: // Equal reports whether t and u represent the same
time instant.
On 2011/11/18 00:00:33, dsymonds wrote:
> Did you want to note that this is different to comparing t and u with ==?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode97
src/pkg/time/time.go:97: // String returns the English name of the month
("January", "Feburary", ...).
On 2011/11/17 22:50:27, bradfitz wrote:
> February

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode98
src/pkg/time/time.go:98: func (m Month) String() string { return months[m] }
On 2011/11/18 06:27:14, r wrote:
> since you crash if m > 12, maybe you should crash if m == 0. you could do that
> by zero-indexing months and subtracting 1 here.

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode200
src/pkg/time/time.go:200: // zones.  On the other hand, I have no problem with
printing a few times
On 2011/11/18 06:27:14, r wrote:
> "I" do not belong here.
> By comparison, it's reasonable to mishandle some times in the year
> -292277022399.

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode201
src/pkg/time/time.go:201: // in the year -292277022399 incorrectly.
On 2011/11/17 23:43:13, gri wrote:
> On the other hand, printing a few times in the year ... incorrectly is
unlikely
> to cause any problems.
> ?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode217
src/pkg/time/time.go:217: // No longer changes.
On 2011/11/17 23:43:13, gri wrote:
> 2nd line comment by itself is not helpful

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode278
src/pkg/time/time.go:278: // Weekday returns the day of the week specified by t.
On 2011/11/18 06:27:14, r wrote:
> On 2011/11/18 00:00:33, dsymonds wrote:
> > This code is incorrect for dates before 1752, and again before 1582. Perhaps
> > call out that this package only operates on post-1752 dates?
> 
> actually it depends where you live.... and the skip only happened once,
wherever
> you are (mostly)

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode278
src/pkg/time/time.go:278: // Weekday returns the day of the week specified by t.
On 2011/11/18 00:00:33, dsymonds wrote:
> This code is incorrect for dates before 1752, and again before 1582. Perhaps
> call out that this package only operates on post-1752 dates?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode370
src/pkg/time/time.go:370: // in the range [0, 999999999].  It is equivalent to
int(t.Nanoseconds)
On 2011/11/17 22:50:27, bradfitz wrote:
> t.Nanoseconds isn't exported.  not worth documenting publicly?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode390
src/pkg/time/time.go:390: // Duration returns a string representing the duration
in the form "72h3m0.005s".
On 2011/11/17 22:50:27, bradfitz wrote:
> 80

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode516
src/pkg/time/time.go:516: // order to have sub-second precision.
On 2011/11/18 06:27:14, r wrote:
> not happy with this. i'm going to do int64(d.Seconds()) and lose data.

In what case are you worried about losing data?
d.Seconds has a range of +/- 2^33, so the float64 can definitely
represent it accurately.

The computation float64(d) / float64(Second) 
does discard the sub-microsecond resolution during the
float64(d), so it is possible that the rounding would go the
wrong way in boundary cases.  I fixed the computation
to be completely accurate.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode555
src/pkg/time/time.go:555: secondsPerWeek   = 7 * secondsPerDay
On 2011/11/18 06:27:14, r wrote:
> at least up to here, these should perhaps be exported. they show up a lot.

I am a little worried about duplication with the duration constants.
Also secondsPerDay and secondsPerWeek are not accurate across
daylight savings switches so most code using them is wrong
anyway.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode640
src/pkg/time/time.go:640: var daysBefore = [...]int32{
Thanks.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode693
src/pkg/time/time.go:693: // name of the zone (such as "EDT") and its offset in
seconds east of UTC.
On 2011/11/18 06:27:14, r wrote:
> it's been EST everywhere else. (but i prefer CET)

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode729
src/pkg/time/time.go:729: func isleap(year int) bool {
On 2011/11/18 06:27:14, r wrote:
> isLeap

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode739
src/pkg/time/time.go:739: // either of the zones.
On 2011/11/18 06:10:14, iant wrote:
> The last clause is a bit cryptic.  I think the most important point is that
the
> returned time will not match what was passed in, and perhaps the comment
should
> just say that.

I tried to make it clearer.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/time.go#newcode745
src/pkg/time/time.go:745: panic("time.Date: invalid day")
Ian's suggestion creates useful functionality (normalization),
matches the C library, and avoids all the panics, so I did that.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
On 2011/11/18 06:10:14, iant wrote:
> On 2011/11/17 23:43:13, gri wrote:
> > I like Location.
> 
> I'm not crazy about Location, because this is really a timezone, it's not a
> specific location.  I think ZoneInfo is OK since the name is widely used.  I
can
> also think of TimeZoneOffsets or TZOffsets.

I tried Location, and I think it works well.

It is a specific location to a certain precision, namely the geographical
area observing a set of time zones.  It shows up in other code (if at all)
as time.Location, which seems qualified enough.  Also the names used
in the IANA database *are* locations.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:23: cacheStart int64
On 2011/11/17 23:43:13, gri wrote:
> the meaning of these fields is not obvious

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:31: utcoff int    // seconds east of UTC
On 2011/11/18 06:27:14, r wrote:
> nice to get utc capitalized. as written it's a poorly styled name. also nice
to
> have the name remind us of the direction
> 
> secEastOfUTC would be a fine name.

Everywhere else the name is just offset.
I changed this one to offset too.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:32: isdst  bool   // is this zone daylight savings
time?
On 2011/11/18 06:27:14, r wrote:
> isDST bool // is this zone Daylight Savings Time?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:35: // A zonetx represents a single time zone
transition.
On 2011/11/18 06:27:14, r wrote:
> zoneTrans or zoneTransition

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:37: time         int64 // transition time, in seconds
since 1970 GMT
On 2011/11/18 06:27:14, r wrote:
> maybe "when"? "time" is overused in this package, to put it mildly.

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:38: zone         uint8 // the index of the zone that
goes into effect at that time
On 2011/11/18 06:27:14, r wrote:
> i'd call this index.
> 
> i'm fiddling with names, i know, but they have a big effect on the readability
> of some gnarly code, dood.

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:66: // given name and offset as the current zone.
On 2011/11/18 06:27:14, r wrote:
> explain relevance of DST

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:69: name = ""
On 2011/11/18 06:10:14, iant wrote:
> This does nothing.

Indeed!  It used to rewrite to "???" but I changed my mind.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:89: // The returned information gives the name of the
zone (such as "EST"),
On 2011/11/18 06:27:14, r wrote:
> how about using CET or something less parochial as your canonical example?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:105: start = -1 << 60
On 2011/11/18 06:10:14, iant wrote:
> Above you used << 63, here you used << 60.  Makes me wonder why they are
> different.

Done.  Missed during some cleanup.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:120: tx := z.tx
On 2011/11/18 06:27:14, r wrote:
> On 2011/11/17 23:43:13, gri wrote:
> > no sort.Search for speed reasons?
> 
> avoiding dependencies

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:144: if z == nil {
On 2011/11/18 06:27:14, r wrote:
> i've seen this burst too many times. refactor.
> 
> z = getZoneInfo(z) or z = z.getZoneInfo()
> 
> func (z *ZoneInfo) getZoneInfo() *ZoneInfo {
> if z == nil {
>   z = &localZone
> }
> if z == &localZone {
>   localOnce.Do(initLocalZone)
> }
> return z
> }

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:160: // LookupName returns information about the time
zone with
On 2011/11/18 06:27:14, r wrote:
> LookupOffset

Done.

[rsc, 2011-11-18 16:29:47]

On Thu, Nov 17, 2011 at 18:43,  <gri@golang.org> wrote:
> src/pkg/time/format.go:827: }
> why not just
>
> for nbytes < 10 {
>  ns *= 10
>  nbytes++
> }

This code is not new in this CL, so I am going to leave it alone.

Russ

[rsc, 2011-11-18 16:35:33]

A few more comments I missed.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:22: // time.now is implemented in assembly.
On 2011/11/18 06:27:14, r wrote:
> is time.now even referenced here?

No, but people might come here to look for it.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:288: //	Mon Jan _2 15:04:05 -0700 MST 2006
On 2011/11/18 06:27:14, r wrote:
> maybe quote the string?

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/sys.go
File src/pkg/time/sys.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/sys.go#newcode12
src/pkg/time/sys.go:12: // readFile reads and returns the content of the named
file.
On 2011/11/18 06:27:14, r wrote:
> // It's a copy of io.ReadFile, here to avoid the dependency.

Done.

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/tick.go
File src/pkg/time/tick.go (right):

http://codereview.appspot.com/5392041/diff/1056/src/pkg/time/tick.go#newcode17
src/pkg/time/tick.go:17: // send the time, in nanoseconds, with a period of d. 
It adjusts the
On 2011/11/18 06:27:14, r wrote:
> "a period of d" reads oddly.
> with a period specified by the duration argument.

Done.

[r, 2011-11-18 17:37:36]

LGTM

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:528: // corresponding to Jan 1, year 0, 15:04:00 +0000
UTC.
+0000 UTC is redundant

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:551: // Each iteration proceseces one std value.
processes

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/tick.go
File src/pkg/time/tick.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/tick.go#newcode19
src/pkg/time/tick.go:19: // The value of d must be greater than zero; if not,
NewTicker will panic.
s/value of d/duration d/
(first mention of d; give context)

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/tick.go#newcode26
src/pkg/time/tick.go:26: // on the floor until the client catches up.
this comment should  be in the function comment

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode518
src/pkg/time/time.go:518: // order to have sub-second precision.
delete at least the phrase "REVIEW NOTE" and remove yourself from the comment.
there's information here worth keeping, so don't delete the whole thing

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode523
src/pkg/time/time.go:523: // that int64(d.Seconds()) == d.Nanoseconds()p/1e9
s/p//

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode524
src/pkg/time/time.go:524: // even in cases where float64(d) / 1e9 would have
s; / ;/;

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode533
src/pkg/time/time.go:533: // units do not have fixed lengths.
delete

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode663
src/pkg/time/time.go:663: 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 30 +
31,
there's an optical illusion here, making this line look like it curves upwards

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode687
src/pkg/time/time.go:687: // In returns t with the location information set to
loc.
// It panics if loc is nil.
(or you could say nil is UTC and avoid the panic)

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode765
src/pkg/time/time.go:765: // For example, October 42 converts to November 11.
easier to understand
For example, October 32 is November 1.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode775
src/pkg/time/time.go:775: panic("time: missing Location in call to Date")
utc?

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
delete this REVIEW NOTE

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:28: cacheStart int64
// seconds since...?

[rsc, 2011-11-18 18:21:36]

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:528: // corresponding to Jan 1, year 0, 15:04:00 +0000
UTC.
On 2011/11/18 17:37:37, r wrote:
> +0000 UTC is redundant

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:551: // Each iteration proceseces one std value.
On 2011/11/18 17:37:37, r wrote:
> processes

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/tick.go
File src/pkg/time/tick.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/tick.go#newcode19
src/pkg/time/tick.go:19: // The value of d must be greater than zero; if not,
NewTicker will panic.
On 2011/11/18 17:37:37, r wrote:
> s/value of d/duration d/
> (first mention of d; give context)

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/tick.go#newcode26
src/pkg/time/tick.go:26: // on the floor until the client catches up.
On 2011/11/18 17:37:37, r wrote:
> this comment should  be in the function comment

Done:

NewTicker returns a new Ticker containing a channel that will send the
time, in nanoseconds, with a period specified by the duration argument.
It adjusts the intervals or drops ticks to make up for slow receivers.
The duration d must be greater than zero; if not, NewTicker will panic.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode518
src/pkg/time/time.go:518: // order to have sub-second precision.
On 2011/11/18 17:37:37, r wrote:
> delete at least the phrase "REVIEW NOTE" and remove yourself from the comment.
> there's information here worth keeping, so don't delete the whole thing

Done:

// These methods return float64 because the dominant
// use case is for printing a floating point number like 1.5s, and
// a truncation to integer would make them not useful in those cases.
// Splitting the integer and fraction ourselves guarantees that
// converting the returned float64 to an integer rounds the same
// way that a pure integer conversion would have, even in cases
// where, say, float64(d.Nanoseconds())/1e9 would have rounded
// differently.

and updated Minutes and Hours to do that too.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode523
src/pkg/time/time.go:523: // that int64(d.Seconds()) == d.Nanoseconds()p/1e9
On 2011/11/18 17:37:37, r wrote:
> s/p//

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode524
src/pkg/time/time.go:524: // even in cases where float64(d) / 1e9 would have
On 2011/11/18 17:37:37, r wrote:
> s; / ;/;

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode533
src/pkg/time/time.go:533: // units do not have fixed lengths.
On 2011/11/18 17:37:37, r wrote:
> delete

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode687
src/pkg/time/time.go:687: // In returns t with the location information set to
loc.
On 2011/11/18 17:37:37, r wrote:
> // It panics if loc is nil.
> (or you could say nil is UTC and avoid the panic)

I added the comment.  I have been trying to force callers to be 
explicit about saying time.Local or time.UTC instead of 'nil',
since it is a point on which there was confusion.

Separately, I am also trying to preserve the property mentioned
in the Time definition that only the zero Time has loc == nil.
If we ever do need to change the internal representation, that will
be a convenient way to detect the zero time.  This does not
imply that the API must reject nil, of course (it could rewrite
to UTC on the way in), but it still seems best to be explicit.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode765
src/pkg/time/time.go:765: // For example, October 42 converts to November 11.
On 2011/11/18 17:37:37, r wrote:
> easier to understand
> For example, October 32 is November 1.

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/time.go#newcode775
src/pkg/time/time.go:775: panic("time: missing Location in call to Date")
On 2011/11/18 17:37:37, r wrote:
> utc?

Added panic to doc comment.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
On 2011/11/18 17:37:37, r wrote:
> delete this REVIEW NOTE

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:12: // Maybe Location?
On 2011/11/18 17:37:37, r wrote:
> delete this REVIEW NOTE

Done.

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:28: cacheStart int64
On 2011/11/18 17:37:37, r wrote:
> // seconds since...?

Done.

[iant, 2011-11-18 23:17:04]

LGTM

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/1112/src/pkg/time/zoneinfo.go#newc...
src/pkg/time/zoneinfo.go:119: end = 1 << 60
s/60/63/ ?

[rsc, 2011-11-21 18:12:35]

I cut the dependencies from format.go:

http://codereview.appspot.com/5392041/diff2/3033:5035/src/pkg/time/format.go

[r, 2011-11-21 18:15:27]

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:258: func itoa(x int) string {
worth saying these reproduce functionality from strconv but avoid the
dependency, that we want this to be nearly a leaf package

[gri, 2011-11-21 18:29:08]

FYI

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:264: neg := x < 0
get rid of neg (can use x since it's never modified)

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:266: if neg {
if x < 0

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:274: if neg {
if x < 0

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:285: neg := false
i := 0 and get rid of neg

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:287: neg = true
I would get rid of neg and just set i to 1 here

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:293: for i := 0; i < len(s); i++ {
use i from outside

http://codereview.appspot.com/5392041/diff/5035/src/pkg/time/format.go#newcod...
src/pkg/time/format.go:304: if neg {
if s[0] == '-'

[rsc, 2011-11-28 18:44:42]

On Mon, Nov 21, 2011 at 13:29,  <gri@golang.org> wrote:
> FYI

Applied these fixes.  Thanks.

[gri, 2011-11-28 22:35:15]

LGTM

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/format.go#newco...
src/pkg/time/format.go:258: func itoa(x int) string {
Is there a comment somewhere explaining that these are re-implemented here so
that we are not depending on strconv (assuming that is the reason)? If not, I
think there should be.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:38: // A zoneTrans represents a single time zone
transition.
maybe s/zoneTrans/transition/ ?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:169: // The files total 200 kB.
kB seems right (as opposed to KB), but then again we write MB (and not mB)...

[dsymonds, 2011-11-28 22:57:48]

FYI

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:169: // The files total 200 kB.
On 2011/11/28 22:35:15, gri wrote:
> kB seems right (as opposed to KB), but then again we write MB (and not mB)...

"k" and "M" (different cases) are fine; they are the standard SI prefixes.
  http://en.wikipedia.org/wiki/SI_prefix#List_of_SI_prefixes

[dsymonds, 2011-11-29 00:01:39]

LGTM

Just some final nit-picky comments.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go
File src/pkg/time/time.go (right):

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode22
src/pkg/time/time.go:22: // gives a simple way of detecting an uninitialized
time.
We normally avoid "uninitialized" right? That has dire implications in other
languages that don't apply to Go, but can still throw people. How about s/an
uninitialized/a zero/?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode25
src/pkg/time/time.go:25: // presentation form of the time, such as in the
Format, Hour, and Year methods.
The use of the word "presentation" in this context tripped up me and others in
the initial API review; it's technically correct, but some of us jump to the
wrong conclusions (e.g. it's the string representation). Can you avoid it here?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode86
src/pkg/time/time.go:86: var months = []string{
[...]

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode117
src/pkg/time/time.go:117: var days = []string{
[...]

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode137
src/pkg/time/time.go:137: // non-negative year even in time zones west of UTC,
unlike 1-1-0
there is no year 0.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode188
src/pkg/time/time.go:188: // uses a year equal to 1 mod 400, and that is no more
than 2⁶³ seconds
those superscript characters display oddly in the web browser font I'm using
right now. Replace that with 2^63?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode389
src/pkg/time/time.go:389: Hour                 = 60 * Minute
Why stop at hour? I'm sure there's a good reason; it should probably be
documented here.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode394
src/pkg/time/time.go:394: // second format use a smaller unit—milli-, micro-, or
nanoseconds—to ensure
dashes are used here as both emdashes and hyphens. It's a little confusing to
parse. How about making this a parenthetical instead?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode495
src/pkg/time/time.go:495: // fmtInt formats v into the tail of buf[:w].
what's w here?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode696
src/pkg/time/time.go:696: panic("time: missing Location in call to Time.In")
t.In(nil) seems reasonable to have as an alias for t.UTC(). I could see a
*Location being passed around, and it'd just be tedious to do a nil check switch
between calling UTC and In.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode723
src/pkg/time/time.go:723: // UnixNano returns the Unix time, the number of
nanoseconds since January 1, 1970 UTC.
wrap

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode780
src/pkg/time/time.go:780: // Date panics if loc is nil.
that's a shame. Why not just default to UTC in thiat case?

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go
File src/pkg/time/zoneinfo.go (right):

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:12: type Location struct {
I agree with Ian that this is an odd name. time.Zone has a nice ring to it.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:46: var UTC *Location = &utcLoc
unnecessary "*Location"

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:48: var utcLoc = Location{name: "UTC"}
why the two vars? Why not just this?
  var UTC = &Location{name: "UTC"}

I must be overlooking something.

http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
src/pkg/time/zoneinfo.go:93: func (l *Location) Lookup(sec int64) (name string,
offset int, isDST bool, start, end int64) {
this is a lot of return parameters. How about a little struct instead?

[rsc, 2011-11-29 01:48:31]

Thanks for the comments.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode25
> src/pkg/time/time.go:25: // presentation form of the time, such as in
> the Format, Hour, and Year methods.
> The use of the word "presentation" in this context tripped up me and
> others in the initial API review; it's technically correct, but some of
> us jump to the wrong conclusions (e.g. it's the string representation).
> Can you avoid it here?

The rest of the sentence should make very clear that
methods like Hour and Year count as presentation.
I added that in response to the initial confusion.
I think having a term for the general class of affected
method is important, and presentation seems like a good one.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode137
> src/pkg/time/time.go:137: // non-negative year even in time zones west
> of UTC, unlike 1-1-0
> there is no year 0.

There was no year 1 in this calendar either.
It didn't start for another millennium (actually more).
Are you suggesting that the routines should not
return a year zero?  There's no year -1 either,
and yet they handle that too.  It's an unambiguous
extension of the existing calendar.  Note also that
this comment is not in the public documentaton.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode188
> src/pkg/time/time.go:188: // uses a year equal to 1 mod 400, and that is
> no more than 2⁶³ seconds
> those superscript characters display oddly in the web browser font I'm
> using right now. Replace that with 2^63?

This too is in the internal documentation, so it is not
such a big deal (maybe worth finding a new font, though).
If it were showing up in public documentation I would be
more concerned.  I try to avoid writing 2^63 because
in Go 2^63 = 61, and I've corrected that mistake in a
handful of code reviews.  We've got Unicode available:
why not use it?

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode389
> src/pkg/time/time.go:389: Hour                 = 60 * Minute
> Why stop at hour? I'm sure there's a good reason; it should probably be
> documented here.

The reason is that hours are always 60 minutes
(ignoring leap seconds, as we do) but days are
not always 24 hours.  If we had Day, then people
would be surprised when t.Hour() != t.Add(1*time.Day).Hour().

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode394
> src/pkg/time/time.go:394: // second format use a smaller unit—milli-,
> micro-, or nanoseconds—to ensure
> dashes are used here as both emdashes and hyphens. It's a little
> confusing to parse. How about making this a parenthetical instead?

Sure.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode495
> src/pkg/time/time.go:495: // fmtInt formats v into the tail of buf[:w].
> what's w here?

I think it is supposed to be tail of buf.  I changed the signature
to slice buf on the way in.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode696
> src/pkg/time/time.go:696: panic("time: missing Location in call to
> Time.In")
> t.In(nil) seems reasonable to have as an alias for t.UTC(). I could see
> a *Location being passed around, and it'd just be tedious to do a nil
> check switch between calling UTC and In.

The nil *Location causes a panic when passed to any API.
I would argue that it is just as reasonable to have t.In(nil)
mean Local time, since that is the default from time.Now().
Because it is unclear which nil should mean, I have defined
that nil means neither, for maximum clarity.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
> src/pkg/time/zoneinfo.go:12: type Location struct {
> I agree with Ian that this is an odd name. time.Zone has a nice ring to
> it.

Yes, except that this is not a time zone.  Many people who hear
time zone think "eastern standard time" or "eastern daylight time"
but a Location is actually a map from time instants to the zone in
use at that time.

If you think about how people choose time zones in any OS installation,
they do so by clicking on a map or picking the name of a place from a
drop down list, and somehow that doesn't bother people.  Even the
standard zoneinfo files are all named for locations.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
> src/pkg/time/zoneinfo.go:46: var UTC *Location = &utcLoc
> unnecessary "*Location"
>
>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
> src/pkg/time/zoneinfo.go:48: var utcLoc = Location{name: "UTC"}
> why the two vars? Why not just this?
>  var UTC = &Location{name: "UTC"}
>
> I must be overlooking something.

I will investigate and probably fix this.  Originally Location was an
interface and &utcLoc an implementation of that interface, so I
suspect this is just a dreg.

>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
> src/pkg/time/zoneinfo.go:93: func (l *Location) Lookup(sec int64) (name
> string, offset int, isDST bool, start, end int64) {
> this is a lot of return parameters. How about a little struct instead?

I thought about it, but that struct would be the time.Zone, which I think
would cause more confusion than it would eliminate.  Almost no one
is supposed to need to call Location.Lookup (in fact, it could be an
unexported method; hmm, that's probably a good idea).

Russ

[dsymonds, 2011-11-29 02:00:04]

On Tue, Nov 29, 2011 at 12:48 PM, Russ Cox <rsc@golang.org> wrote:

>>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode137
>> src/pkg/time/time.go:137: // non-negative year even in time zones west
>> of UTC, unlike 1-1-0
>> there is no year 0.
>
> There was no year 1 in this calendar either.
> It didn't start for another millennium (actually more).
> Are you suggesting that the routines should not
> return a year zero?  There's no year -1 either,
> and yet they handle that too.  It's an unambiguous
> extension of the existing calendar.  Note also that
> this comment is not in the public documentaton.

In the Gregorian calendar, the day before 1 January 1 AD is 31
December 1 BC. Sure, it was backdated, but it's the current calendar
system. -1 seems a reasonable approximation of 1 BC. I don't think
these routines should be returning a year 0. It's an edge case, and
it's not public, so I'm not fussed; just flagging it.

>>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode188
>> src/pkg/time/time.go:188: // uses a year equal to 1 mod 400, and that is
>> no more than 2⁶³ seconds
>> those superscript characters display oddly in the web browser font I'm
>> using right now. Replace that with 2^63?
>
> This too is in the internal documentation, so it is not
> such a big deal (maybe worth finding a new font, though).
> If it were showing up in public documentation I would be
> more concerned.  I try to avoid writing 2^63 because
> in Go 2^63 = 61, and I've corrected that mistake in a
> handful of code reviews.  We've got Unicode available:
> why not use it?

Just that in at least my browser, the Unicode code points for the
superscript "6" and "3" are very small, and are different sizes. But,
again, internal, so I'm not fussed.

>>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/time.go#newcode389
>> src/pkg/time/time.go:389: Hour                 = 60 * Minute
>> Why stop at hour? I'm sure there's a good reason; it should probably be
>> documented here.
>
> The reason is that hours are always 60 minutes
> (ignoring leap seconds, as we do) but days are
> not always 24 hours.  If we had Day, then people
> would be surprised when t.Hour() != t.Add(1*time.Day).Hour().

You're talking about timezone transitions? If so, the same applies to
Hour, because there's plenty of timezones that aren't integral hours
from UTC.

>>
http://codereview.appspot.com/5392041/diff/11004/src/pkg/time/zoneinfo.go#new...
>> src/pkg/time/zoneinfo.go:12: type Location struct {
>> I agree with Ian that this is an odd name. time.Zone has a nice ring to
>> it.
>
> Yes, except that this is not a time zone.  Many people who hear
> time zone think "eastern standard time" or "eastern daylight time"
> but a Location is actually a map from time instants to the zone in
> use at that time.
>
> If you think about how people choose time zones in any OS installation,
> they do so by clicking on a map or picking the name of a place from a
> drop down list, and somehow that doesn't bother people.  Even the
> standard zoneinfo files are all named for locations.

Okay, fair enough. But in that case, you've got "zone" sprinkled
throughout the (public) docs in time.go that should then be
"location".


Dave.

[rsc, 2011-11-30 16:20:29]

On Mon, Nov 28, 2011 at 21:00, David Symonds <dsymonds@golang.org> wrote:
> In the Gregorian calendar, the day before 1 January 1 AD is 31
> December 1 BC. Sure, it was backdated, but it's the current calendar
> system. -1 seems a reasonable approximation of 1 BC.

0 seems an even more reasonable approximation of 1 BC.

Russ

[rsc, 2011-11-30 16:23:44]

On Mon, Nov 28, 2011 at 21:00, David Symonds <dsymonds@golang.org> wrote:
> Okay, fair enough. But in that case, you've got "zone" sprinkled
> throughout the (public) docs in time.go that should then be
> "location".

Great point.  I went through all the places that said zone
and changed the ones that should have said location.

Russ

[rsc, 2011-11-30 16:59:50]

*** Submitted as http://code.google.com/p/go/source/detail?r=5bd1bc51fc04 ***

time: new Time, Duration, ZoneInfo types

R=r, bradfitz, gri, dsymonds, iant
CC=golang-dev
http://codereview.appspot.com/5392041