Add microsecond support to DateTime.

Add microsecond support to DateTime.

R=iposva@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/6c84c42e671cdb5f03132c89e0a926a37dcdba2e

[floitsch, 2015-12-03 08:18:10]

floitsch@google.com changed reviewers:
+ iposva@google.com, lrn@google.com

[floitsch, 2015-12-03 08:18:11]

I first tried to have a separate microsecond field, but that turned out to be
quite annoying. In the end, I went with a _value field that can be either micro
or millisecondSinceEpoch.

I still have to add a few tests and fix the ones that are now failing (because
they rely on rounding of the microseconds).

[Lasse Reichstein Nielsen, 2015-12-03 09:16:22]

lgtm

https://codereview.chromium.org/1493033003/diff/20001/runtime/lib/date_patch....
File runtime/lib/date_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:84: static List _computeUpperPart(int localUs) {
Us -> Microseconds (everywhere, or at least where capitalized).
Maybe just "Micros", but not "Us".
The "us" works when placed right next to "ms", but otherwise it isn't readable.
It looks like either a word (object case of "we") or the United States. It
doesn't look like the SI prefix for micro ("μ") at all, and U isn't pronounced
anywhere near "mu" (which is spelled and pronounced "my" in Danish, so there's
not even a "u" in it :).

https://codereview.chromium.org/1493033003/diff/20001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:214: * * [:this.hour == new
DateTime.fromMicrosecondsSinceEpoch(t, true).hour:],
While you are here: [::] -> ``.

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:190: {bool isUtc: false})
indentation is wrong.

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:257: _value +
duration.inMilliseconds, isUtc: isUtc);
This means that microseconds are ignored, not rounded like they are in the
constructor. This is now visible.
  
  var now = new DateTime.now();
  var then = now.add(const Duration(microseconds: 1))
  print(now == then);  // True in dart2js, false in VM.

I guess that's the cost of doing business with JS.

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/core/date_time....
File sdk/lib/core/date_time.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/core/date_time....
sdk/lib/core/date_time.dart:446: int get hashCode => _value;
That's not necessarily a good hash code. Could we mangle the bits a little?

[floitsch, 2015-12-04 08:11:31]

https://codereview.chromium.org/1493033003/diff/20001/runtime/lib/date_patch....
File runtime/lib/date_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:84: static List _computeUpperPart(int localUs) {
On 2015/12/03 09:16:22, Lasse Reichstein Nielsen wrote:
> Us -> Microseconds (everywhere, or at least where capitalized).
> Maybe just "Micros", but not "Us".
> The "us" works when placed right next to "ms", but otherwise it isn't
readable.
> It looks like either a word (object case of "we") or the United States. It
> doesn't look like the SI prefix for micro ("μ") at all, and U isn't pronounced
> anywhere near "mu" (which is spelled and pronounced "my" in Danish, so there's
> not even a "u" in it :).
> 

Agreed. One of these things that feel natural when you write them, but don't
make sense when coming back.
Changed to micros.

https://codereview.chromium.org/1493033003/diff/20001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:214: * * [:this.hour == new
DateTime.fromMicrosecondsSinceEpoch(t, true).hour:],
On 2015/12/03 09:16:22, Lasse Reichstein Nielsen wrote:
> While you are here: [::] -> ``.

Done.

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:190: {bool isUtc: false})
On 2015/12/03 09:16:22, Lasse Reichstein Nielsen wrote:
> indentation is wrong.

Done.

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:257: _value +
duration.inMilliseconds, isUtc: isUtc);
On 2015/12/03 09:16:22, Lasse Reichstein Nielsen wrote:
> This means that microseconds are ignored, not rounded like they are in the
> constructor. This is now visible.
>   
>   var now = new DateTime.now();
>   var then = now.add(const Duration(microseconds: 1))
>   print(now == then);  // True in dart2js, false in VM.
> 
> I guess that's the cost of doing business with JS.

Do you want me to round them?
Unless I made a mistake this is the behavior we had before this patch.

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/core/date_time....
File sdk/lib/core/date_time.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/core/date_time....
sdk/lib/core/date_time.dart:446: int get hashCode => _value;
On 2015/12/03 09:16:22, Lasse Reichstein Nielsen wrote:
> That's not necessarily a good hash code. Could we mangle the bits a little?

(_value ^ (_value >> 30)) & 0x3FFFFFFF;

[Lasse Reichstein Nielsen, 2015-12-04 08:15:02]

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:257: _value +
duration.inMilliseconds, isUtc: isUtc);
I think it's probably fine.
Before this change, users wouldn't expect DateTime to understand microseconds,
so having them ignored was expected. Now it looks like they are supported, and
Duration does support them, so it's more surprising when they are lost (I don't
think it makes a difference whether we round or truncate).

[floitsch, 2015-12-08 00:35:48]

ping (ivan).

[kevmoo, 2015-12-08 02:40:56]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2015-12-08 02:40:57]

Please update CHANGELOG w/ this change

[Ivan Posva, 2015-12-08 07:51:41]

LGTM -ip

https://codereview.chromium.org/1493033003/diff/40001/runtime/lib/date_patch....
File runtime/lib/date_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/40001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:10: static int _getCurrentMicros() native
"DateNatives_currentTimeMicros";
This native should be named "DateTime_currentTimeMicros" as we have not had the
DateNatives class in a long time.

https://codereview.chromium.org/1493033003/diff/40001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:13: native "DateNatives_timeZoneName";
ditto here and below.

[floitsch, 2015-12-08 23:27:25]

https://codereview.chromium.org/1493033003/diff/40001/runtime/lib/date_patch....
File runtime/lib/date_patch.dart (right):

https://codereview.chromium.org/1493033003/diff/40001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:10: static int _getCurrentMicros() native
"DateNatives_currentTimeMicros";
On 2015/12/08 07:51:41, Ivan Posva wrote:
> This native should be named "DateTime_currentTimeMicros" as we have not had
the
> DateNatives class in a long time.

Done.

https://codereview.chromium.org/1493033003/diff/40001/runtime/lib/date_patch....
runtime/lib/date_patch.dart:13: native "DateNatives_timeZoneName";
On 2015/12/08 07:51:41, Ivan Posva wrote:
> ditto here and below.

Done.

[floitsch, 2015-12-08 23:46:51]

Description was changed from

==========
Add microsecond support to DateTime.
==========

to

==========
Add microsecond support to DateTime.

R=iposva@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6c84c42e671cdb5f03132c89e0a926a37dcdba2e
==========

[floitsch, 2015-12-08 23:46:52]

Committed patchset #4 (id:60001) manually as
6c84c42e671cdb5f03132c89e0a926a37dcdba2e (presubmit successful).