Updated VM and JS versions of Date to allow underflow and overflow.

Updated VM and JS versions of Date to allow underflow and overflow.

Added many tests to check everything is working as expected.

BUG=2771
Committed: https://code.google.com/p/dart/source/detail?r=11453

[floitsch, 2012-08-06 21:00:58]

FYI.

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (left):

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart#...
runtime/lib/date.dart:358: days += _DAYS_UNTIL_MONTH[_isLeapYear(years) ? 1 :
0][month - 1];
Isn't it enough to:
years += (month - 1) ~/ 12;
int clampedMonth = (month - 1).remainder(12);
if (clampedMonth < 0) { years--; clampedMonth += 12; }

The hour, minute, second and milliseconds are correctly added later and don't
depend on the month/year anyways.

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart#...
runtime/lib/date.dart:250: const [0, 31, 60, 91, 121, 152, 182, 213, 244, 274,
305, 335, 366]];
80chars.

https://chromiumcodereview.appspot.com/10832166/diff/1/tests/corelib/date_tim...
File tests/corelib/date_time_test.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/1/tests/corelib/date_tim...
tests/corelib/date_time_test.dart:429: var dt = new Date(dtBase.year,
dtBase.month, dtBase.day, dtBase.hour, dtBase.minute, dtBase.second, 1000);
80 chars.

[dominich, 2012-08-06 21:20:58]

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (left):

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart#...
runtime/lib/date.dart:358: days += _DAYS_UNTIL_MONTH[_isLeapYear(years) ? 1 :
0][month - 1];
On 2012/08/06 21:00:58, floitsch wrote:
> Isn't it enough to:
> years += (month - 1) ~/ 12;
> int clampedMonth = (month - 1).remainder(12);
> if (clampedMonth < 0) { years--; clampedMonth += 12; }

No - clampedMonth could be -50 in which case years should be decremented
further.

> 
> The hour, minute, second and milliseconds are correctly added later and don't
> depend on the month/year anyways.

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart#...
runtime/lib/date.dart:250: const [0, 31, 60, 91, 121, 152, 182, 213, 244, 274,
305, 335, 366]];
On 2012/08/06 21:00:58, floitsch wrote:
> 80chars.

Done.

https://chromiumcodereview.appspot.com/10832166/diff/1/tests/corelib/date_tim...
File tests/corelib/date_time_test.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/1/tests/corelib/date_tim...
tests/corelib/date_time_test.dart:429: var dt = new Date(dtBase.year,
dtBase.month, dtBase.day, dtBase.hour, dtBase.minute, dtBase.second, 1000);
On 2012/08/06 21:00:58, floitsch wrote:
> 80 chars.

Done.

[floitsch, 2012-08-07 09:10:07]

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (left):

https://chromiumcodereview.appspot.com/10832166/diff/1/runtime/lib/date.dart#...
runtime/lib/date.dart:358: days += _DAYS_UNTIL_MONTH[_isLeapYear(years) ? 1 :
0][month - 1];
On 2012/08/06 21:20:58, dominich wrote:
> On 2012/08/06 21:00:58, floitsch wrote:
> > Isn't it enough to:
> > years += (month - 1) ~/ 12;
> > int clampedMonth = (month - 1).remainder(12);
> > if (clampedMonth < 0) { years--; clampedMonth += 12; }
> 
> No - clampedMonth could be -50 in which case years should be decremented
> further.
I don't see how this could happen since it is .remainder(12).
> 
> > 
> > The hour, minute, second and milliseconds are correctly added later and
don't
> > depend on the month/year anyways.
>

[dominich, 2012-08-13 19:26:35]

http://codereview.chromium.org/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (left):

http://codereview.chromium.org/10832166/diff/1/runtime/lib/date.dart#oldcode358
runtime/lib/date.dart:358: days += _DAYS_UNTIL_MONTH[_isLeapYear(years) ? 1 :
0][month - 1];
On 2012/08/07 09:10:07, floitsch wrote:
> On 2012/08/06 21:20:58, dominich wrote:
> > On 2012/08/06 21:00:58, floitsch wrote:
> > > Isn't it enough to:
> > > years += (month - 1) ~/ 12;
> > > int clampedMonth = (month - 1).remainder(12);
> > > if (clampedMonth < 0) { years--; clampedMonth += 12; }
> > 
> > No - clampedMonth could be -50 in which case years should be decremented
> > further.
> I don't see how this could happen since it is .remainder(12).

i've just realised - i'm not sure which bit of the code you're suggesting
replacing. The comment is under the day loop (that needs to evaluate every
month) and the code above that changes the years/months is just a divide and
floor, rather than the remainder, and doesn't have the branch.

why would your suggestion be better, and which bit of the code is it suggested
to improve?

> > 
> > > 
> > > The hour, minute, second and milliseconds are correctly added later and
> don't
> > > depend on the month/year anyways.
> > 
>

[floitsch, 2012-08-13 19:41:34]

http://codereview.chromium.org/10832166/diff/1/runtime/lib/date.dart
File runtime/lib/date.dart (left):

http://codereview.chromium.org/10832166/diff/1/runtime/lib/date.dart#oldcode358
runtime/lib/date.dart:358: days += _DAYS_UNTIL_MONTH[_isLeapYear(years) ? 1 :
0][month - 1];
On 2012/08/13 19:26:35, dominich wrote:
> On 2012/08/07 09:10:07, floitsch wrote:
> > On 2012/08/06 21:20:58, dominich wrote:
> > > On 2012/08/06 21:00:58, floitsch wrote:
> > > > Isn't it enough to:
> > > > years += (month - 1) ~/ 12;
> > > > int clampedMonth = (month - 1).remainder(12);
> > > > if (clampedMonth < 0) { years--; clampedMonth += 12; }
> > > 
> > > No - clampedMonth could be -50 in which case years should be decremented
> > > further.
> > I don't see how this could happen since it is .remainder(12).
> 
> i've just realised - i'm not sure which bit of the code you're suggesting
> replacing. The comment is under the day loop (that needs to evaluate every
> month) and the code above that changes the years/months is just a divide and
> floor, rather than the remainder, and doesn't have the branch.
> 
> why would your suggestion be better, and which bit of the code is it suggested
> to improve?
> 
> > > 
> > > > 
> > > > The hour, minute, second and milliseconds are correctly added later and
> > don't
> > > > depend on the month/year anyways.
> > > 
> > 
> 
Unless I'm wrong you don't need to do any loops. The beginning of this function
could be:
_brokenDown...(int year, ...) {
  // First compute the seconds ...
  years += (month - 1) ~/ 12;
  int clampedMonth = (month - 1).remainder(12);
  if (clampedMonth < 0) { years--; clampedMonth += 12; }
  int days = day - 1;
  days += _DAYS_UNTIL_MONTH[_isLeapYear(year) ? 1 : 0][clampedMonth - 1];
  days += _dayFromYear(year);
  ... // same as before.

[dominich, 2012-08-13 21:55:27]

Tweaked a bit, but all tests pass in dart2js/d8 and none/vm.

[floitsch, 2012-08-14 08:31:38]

Getting there :)

https://chromiumcodereview.appspot.com/10832166/diff/9001/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/9001/runtime/lib/date.da...
runtime/lib/date.dart:348: second += (millisecond / 1000).floor().toInt();
You shouldn't need to look at the hours, minutes, seconds and milliseconds. We
don't care if they over or underflow since we end up adding them anyways.
Lines 347 to 355 can be removed.

https://chromiumcodereview.appspot.com/10832166/diff/9001/runtime/lib/date.da...
runtime/lib/date.dart:357: month = (month - 1) % 12 + 1;
Instead of dealing with +/- 1 all over the place do it once in the beginning
with a comment. For example:
// Dart's month is 1-indexed. Remove 1 to make it zero-based to simplify
computations.
month--;

[dominich, 2012-08-14 16:49:20]

https://chromiumcodereview.appspot.com/10832166/diff/9001/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/9001/runtime/lib/date.da...
runtime/lib/date.dart:348: second += (millisecond / 1000).floor().toInt();
On 2012/08/14 08:31:38, floitsch wrote:
> You shouldn't need to look at the hours, minutes, seconds and milliseconds. We
> don't care if they over or underflow since we end up adding them anyways.
> Lines 347 to 355 can be removed.

Of course - good point :)

https://chromiumcodereview.appspot.com/10832166/diff/9001/runtime/lib/date.da...
runtime/lib/date.dart:357: month = (month - 1) % 12 + 1;
On 2012/08/14 08:31:38, floitsch wrote:
> Instead of dealing with +/- 1 all over the place do it once in the beginning
> with a comment. For example:
> // Dart's month is 1-indexed. Remove 1 to make it zero-based to simplify
> computations.
> month--;

Done.

[floitsch, 2012-08-15 08:44:26]

LGTM.

I added Lasse to make sure all relevant files are updated. He is currently
refactoring the library, and knows if there are other copies of the code.

https://chromiumcodereview.appspot.com/10832166/diff/12002/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/12002/runtime/lib/date.d...
runtime/lib/date.dart:250: const [0, 31, 59, 90, 120, 151, 181, 212, 243, 273,
304, 334, 365],
No need for the additional days anymore.

https://chromiumcodereview.appspot.com/10832166/diff/12002/tests/corelib/date...
File tests/corelib/date_time_test.dart (left):

https://chromiumcodereview.appspot.com/10832166/diff/12002/tests/corelib/date...
tests/corelib/date_time_test.dart:273: dt.hour, dt.minute, 0, -1));
This test should still fail. It failed because of the -1, and now it should fail
because the date is too far in the past.
There is just a small bug in the test: the new Date must be isUtc.
Expect.throws(() => new Date(dt.year, dt.month, dt.day, dt.hour, dt.minute, 0,
-1, isUtc: true)));

[dominich, 2012-08-17 16:38:33]

https://chromiumcodereview.appspot.com/10832166/diff/12002/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/12002/runtime/lib/date.d...
runtime/lib/date.dart:250: const [0, 31, 59, 90, 120, 151, 181, 212, 243, 273,
304, 334, 365],
On 2012/08/15 08:44:26, floitsch wrote:
> No need for the additional days anymore.

Done.

https://chromiumcodereview.appspot.com/10832166/diff/12002/tests/corelib/date...
File tests/corelib/date_time_test.dart (left):

https://chromiumcodereview.appspot.com/10832166/diff/12002/tests/corelib/date...
tests/corelib/date_time_test.dart:273: dt.hour, dt.minute, 0, -1));
On 2012/08/15 08:44:26, floitsch wrote:
> This test should still fail. It failed because of the -1, and now it should
fail
> because the date is too far in the past.
> There is just a small bug in the test: the new Date must be isUtc.
> Expect.throws(() => new Date(dt.year, dt.month, dt.day, dt.hour, dt.minute, 0,
> -1, isUtc: true)));

Done.

[dominich, 2012-08-23 16:32:32]

lrn: Can I get a LG?

[Lasse Reichstein Nielsen, 2012-08-24 08:39:02]

LGTM if fixed :)

https://chromiumcodereview.appspot.com/10832166/diff/5004/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/5004/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:468: if (years <= 0 || years <
100) return patchUpY2K(value, years, isUtc);
Should that be "years >= 0 && ..."? And what's with "y2k"? It's a Y1.9K hack :)

I don't think this works correctly. If the year is in the range 0..99, the
constructor/Date.UTC functions add 1900. That's not divisible by 400, so it
might hit a wrong leap day because year 0 is a leap year, but 1900 is not.
Try  valueFromDecomposeDate(0, 2, 29, 0, 0, 0, 0, false) 
It should create a Date which is the 1st of Match 1900, and then set the year to
0, but there is a 29th of February in year 0.
 
Instead, if year is in the range 0..99, add 400 before calling the constructor,
and subtract them again afterwards.

Something like:

var jsYears = years;
if (0 <= years && years < 100) {
  jsYears += 400;
}
... create value from jsYears...
if (0 <= years && years < 100) {
  value -= 400 * 365.2425 * 864e5; // Do precalculate that as an int.
}

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
runtime/lib/date.dart:10: DateImplementation(int year,
Please remove "Implementation". It's never a good name. Ever. I mean it.
Find something that distinguishes this from any other Date instance. Call it
VMDate for all I care. Just. Not. "Implementation"!
Or, preferably, combine it with Date.

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
runtime/lib/date.dart:337: static bool _isLeapYear(y) {
Add type for "y". I think it's always int.

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
runtime/lib/date.dart:347: // simplify some calculations.
Why reference JavaScript here? This is VM code, right?
It's simply simpler to work with zero based months and days.

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
runtime/lib/date.dart:352: int clampedMonth = month.remainder(12);
How can this be necessary when %12 above gives a value in the range 0..11?

https://chromiumcodereview.appspot.com/10832166/diff/5004/tests/corelib/date_...
File tests/corelib/date_time_test.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/5004/tests/corelib/date_...
tests/corelib/date_time_test.dart:805: 
Please add test that leap-days are correct for any normal (non 0..99) period of
400 years plus the years 0..99.
Check at least:
  new Date(year, 2, 29) is in February in leap years and March in non-leap
years, and
  new Date(year, 3, 0)  is always in February and is the 29th in leap years and
28th in non-leap years.

[Lasse Reichstein Nielsen, 2012-08-24 08:40:59]

But adding the other people who are moving library files around: Mads, Anders,
any problems with the file layout?

[Mads Ager (google), 2012-08-24 08:43:16]

The files are probably in new locations but the code should be the same.

[floitsch, 2012-08-24 22:32:48]

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.dart
File runtime/lib/date.dart (right):

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
runtime/lib/date.dart:347: // simplify some calculations.
On 2012/08/24 08:39:02, Lasse Reichstein Nielsen wrote:
> Why reference JavaScript here? This is VM code, right?
> It's simply simpler to work with zero based months and days.
My fault.

https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
runtime/lib/date.dart:352: int clampedMonth = month.remainder(12);
On 2012/08/24 08:39:02, Lasse Reichstein Nielsen wrote:
> How can this be necessary when %12 above gives a value in the range 0..11?
Agreed. Either use 
year += month ~/ 12;
clampedMonth = month.remainder(12);
if (...)

or use
year += (month / 12).floor().toInt();
month = month % 12;

[floitsch, 2012-08-24 22:34:09]

On 2012/08/24 08:39:02, Lasse Reichstein Nielsen wrote:
> LGTM if fixed :)
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/lib/compiler/implem...
> File lib/compiler/implementation/lib/js_helper.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/lib/compiler/implem...
> lib/compiler/implementation/lib/js_helper.dart:468: if (years <= 0 || years <
> 100) return patchUpY2K(value, years, isUtc);
> Should that be "years >= 0 && ..."? And what's with "y2k"? It's a Y1.9K hack
:)
> 
> I don't think this works correctly. If the year is in the range 0..99, the
> constructor/Date.UTC functions add 1900. That's not divisible by 400, so it
> might hit a wrong leap day because year 0 is a leap year, but 1900 is not.
> Try  valueFromDecomposeDate(0, 2, 29, 0, 0, 0, 0, false) 
> It should create a Date which is the 1st of Match 1900, and then set the year
to
> 0, but there is a 29th of February in year 0.
>  
> Instead, if year is in the range 0..99, add 400 before calling the
constructor,
> and subtract them again afterwards.
> 
> Something like:
> 
> var jsYears = years;
> if (0 <= years && years < 100) {
>   jsYears += 400;
> }
> ... create value from jsYears...
> if (0 <= years && years < 100) {
>   value -= 400 * 365.2425 * 864e5; // Do precalculate that as an int.
> }
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.dart
> File runtime/lib/date.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
> runtime/lib/date.dart:10: DateImplementation(int year,
> Please remove "Implementation". It's never a good name. Ever. I mean it.
> Find something that distinguishes this from any other Date instance. Call it
> VMDate for all I care. Just. Not. "Implementation"!
> Or, preferably, combine it with Date.
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
> runtime/lib/date.dart:337: static bool _isLeapYear(y) {
> Add type for "y". I think it's always int.
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
> runtime/lib/date.dart:347: // simplify some calculations.
> Why reference JavaScript here? This is VM code, right?
> It's simply simpler to work with zero based months and days.
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/runtime/lib/date.da...
> runtime/lib/date.dart:352: int clampedMonth = month.remainder(12);
> How can this be necessary when %12 above gives a value in the range 0..11?
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/tests/corelib/date_...
> File tests/corelib/date_time_test.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10832166/diff/5004/tests/corelib/date_...
> tests/corelib/date_time_test.dart:805: 
> Please add test that leap-days are correct for any normal (non 0..99) period
of
> 400 years plus the years 0..99.
> Check at least:
>   new Date(year, 2, 29) is in February in leap years and March in non-leap
> years, and
>   new Date(year, 3, 0)  is always in February and is the 29th in leap years
and
> 28th in non-leap years.

Could we please keep this CL small and targeted.
I'm happy to clean the Date class when I come back from vacation, but I don't
think that most changes you request should be combined in this CL. (Aside from
the fact that I don't find it nice to force clean-up tasks on contributors).

[dominich, 2012-08-24 22:57:48]

made the changes to the method i'd already touched.

if you want me to make broader changes i can, but it feels like that should be a
new CL.

[dominich, 2012-08-27 20:50:15]

On 2012/08/24 22:57:48, dominich wrote:
> made the changes to the method i'd already touched.
> 
> if you want me to make broader changes i can, but it feels like that should be
a
> new CL.

ping - is there agreement that this is good to go in as is and the other changes
should be in a different CL?

[Lasse Reichstein Nielsen, 2012-08-28 09:18:30]

On 2012/08/27 20:50:15, dominich wrote:
> On 2012/08/24 22:57:48, dominich wrote:
> > made the changes to the method i'd already touched.
> > 
> > if you want me to make broader changes i can, but it feels like that should
be
> a
> > new CL.
> 
> ping - is there agreement that this is good to go in as is and the other
changes
> should be in a different CL?

Agree. LGTM, and Florian will fix any existing bugs later :)

[dominich, 2012-08-28 21:09:37]

co19 tests are out of date so we need to suppress them.

[floitsch, 2012-08-29 08:39:37]

co19 status file updates LGTM.