make DateParser::TimeComposer handle milliseconds properly...

make DateParser::TimeComposer handle 1-2 digits millisecond values 

see http://code.google.com/p/v8/issues/detail?id=944
This patch makes DateParser::TimeComposer process times that have
millisecond values with only 1 or 2 digits.

Without this patch, Date.parse("2010-11-25T22:02:30.5") returns
1290690150005 and
Date.parse("2010-11-25T22:02:30.5") == Date.parse("2010-11-25T22:02:30.005")
evaluates to true.

With this patch, Date.parse("2010-11-25T22:02:30.5") returns
1290690150500 instead, and
Date.parse("2010-11-25T22:02:30.5") == Date.parse("2010-11-25T22:02:30.005")
evaluates to false.

[sideshowbarker, 2010-11-25 09:33:15]

This is for http://code.google.com/p/v8/issues/detail?id=944 and is only the
second patch for V8 I've ever submitted. I'm happy to make any changes to it
that might be necessary.

[Lasse Reichstein, 2010-11-25 10:17:18]

http://codereview.chromium.org/5336005/diff/3001/src/dateparser.h
File src/dateparser.h (right):

http://codereview.chromium.org/5336005/diff/3001/src/dateparser.h#newcode92
src/dateparser.h:92: int ReadMilliseconds() {
Seems like overkill (and potentially fragile) to read and parse all those
digits, and then throwing them away by dividing.

How about just reading the first up-to-three digits and collect them in a
result. Then, if reading less than three, multiply it by ten until it's properly
aligned (so .5 becomes 500). Then skip any following digits.
This avoids having to check against kMaxInt, and doesn't limit the number of
digits arbitrarily.

http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-944.js (right):

http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-944.js:28: // Checke for proper parsing of ES5
15.9.1.15 (ISO 8601 / RFC 3339) times
"Checke"->"Check".
Technically this is not ES5 15.9.1.15 parsing, since that only specifies
three-digit milliseconds. Say that it's testing an extension, or generalization,
of the spec format.

http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-944.js:30: assertEquals(1290690150500,
Date.parse("2010-11-25T22:02:30.5"));
Also check that we fail with zero digits (i.e. "2010-11-25T22:02:30." gives
NaN).

Also check with more than three digits (since the parser allows it). In
particular, check that
- we truncate values instead of rounding (.1005 gives .100, not .101).
- we accept any number of digits (no reason to be stingy), e.g.
"2010-11-25T22:02:30.99999999999999999999999999999999999999999999999999999999999999999999999999999999999999"
(or more :)

http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-944.js:33: assertEquals(false,
Date.parse("2010-11-25T22:02:30.5") === Date.parse("2010-11-25T22:02:30.005"));
Use assertFalse(...) instead of assertEquals(false, ...) (or feel free to add
assertNotEquals to mjsunit.js).

[sideshowbarker, 2010-11-25 12:40:51]

Thanks for the review. Please see my responses below.

On 2010/11/25 10:17:18, Lasse Reichstein wrote:
> http://codereview.chromium.org/5336005/diff/3001/src/dateparser.h#newcode92
> src/dateparser.h:92: int ReadMilliseconds() {
> How about just reading the first up-to-three digits and collect them in a
> result. Then, if reading less than three, multiply it by ten until it's
properly
> aligned (so .5 becomes 500). Then skip any following digits.

OK, please see the updated patch. I changed it to something like what you
described above, though not exactly. But it seems like the simplest way to do
it. Let me know if I need to tweak it further.

>
http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
> test/mjsunit/regress/regress-944.js:28: // Checke for proper parsing of ES5
> 15.9.1.15 (ISO 8601 / RFC 3339) times
> "Checke"->"Check".
> Technically this is not ES5 15.9.1.15 parsing, since that only specifies
> three-digit milliseconds. Say that it's testing an extension, or
generalization,
> of the spec format.

OK, did that.

>
http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
> test/mjsunit/regress/regress-944.js:30: assertEquals(1290690150500,
> Date.parse("2010-11-25T22:02:30.5"));
> Also check that we fail with zero digits (i.e. "2010-11-25T22:02:30." gives
> NaN).

OK, added.

> Also check with more than three digits (since the parser allows it). In
> particular, check that
> - we truncate values instead of rounding (.1005 gives .100, not .101).

OK, added.

> - we accept any number of digits (no reason to be stingy), e.g.
>
"2010-11-25T22:02:30.99999999999999999999999999999999999999999999999999999999999999999999999999999999999999"

OK, added.

>
http://codereview.chromium.org/5336005/diff/3001/test/mjsunit/regress/regress...
> test/mjsunit/regress/regress-944.js:33: assertEquals(false,
> Date.parse("2010-11-25T22:02:30.5") ===
Date.parse("2010-11-25T22:02:30.005"));
> Use assertFalse(...) instead of assertEquals(false, ...) (or feel free to add
> assertNotEquals to mjsunit.js).

OK, switched to assertFalse(...)

[Lasse Reichstein, 2010-11-25 12:56:05]

That's a good approach to reading decimals. Fewer loops than my suggestion :).

http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h
File src/dateparser.h (right):

http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h#newcode96
src/dateparser.h:96: if (i < 3) n = n + pow(10, 2 - i) * (ch_ - '0');
I'm somewhat opposed to using the standard pow(double, double) to do simple
powers of integers.
How about managing the multiplier manually, i.e., set it to 100 before entering
the loop, and inside the condition, divide it by 10.
(You could even use it as the if-condition, and stop adding when it reaches zero
- or event skipping the conditional entirely, and relying on a zero multiplier
to make the addition do nothing).

http://codereview.chromium.org/5336005/diff/12001/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-944.js (right):

http://codereview.chromium.org/5336005/diff/12001/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-944.js:31: assertEquals(1290722550521,
Date.parse("2010-11-25T22:02:30.521Z"));
Perhaps add some spacing here, before the next comment, purely for readability
(ditto for the following comments).

http://codereview.chromium.org/5336005/diff/12001/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-944.js:38:
assertEquals(Date.parse("2010-11-25T22:02:30.1006Z"),
Date.parse("2010-11-25T22:02:30.100Z"));
Should this be ".1005Z" (and not ".1006Z")?

[Florian Loitsch, 2010-11-25 13:55:53]

http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h
File src/dateparser.h (right):

http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h#newcode96
src/dateparser.h:96: if (i < 3) n = n + pow(10, 2 - i) * (ch_ - '0');
Isn't this simply:
for (int i = 0; IsAsciiDigit(); Next(), i++) {
  if (i < 3) n = n * 10 + (ch_ - '0');
}
return n;
?

[Lasse Reichstein, 2010-11-25 14:10:36]

http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h
File src/dateparser.h (right):

http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h#newcode96
src/dateparser.h:96: if (i < 3) n = n + pow(10, 2 - i) * (ch_ - '0');
Not if there are fewer than three digits. We need .5, .500 and .50012124 to all
map to 500.

[sideshowbarker, 2010-11-25 22:28:32]

On 2010/11/25 12:56:05, Lasse Reichstein wrote:
> http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h#newcode96
> src/dateparser.h:96: if (i < 3) n = n + pow(10, 2 - i) * (ch_ - '0');
> I'm somewhat opposed to using the standard pow(double, double) to do simple
> powers of integers.

Yeah, I guess I was just being lazy. It's certainly overkill here.

> How about managing the multiplier manually, i.e., set it to 100 before
entering
> the loop, and inside the condition, divide it by 10.
> (You could even use it as the if-condition, and stop adding when it reaches
zero
> - or even skipping the conditional entirely, and relying on a zero multiplier
> to make the addition do nothing).

Nice. Thanks -- I wish I had thought of that :) See updated patch -- I have
changed it to the behavior you outlined.

>
http://codereview.chromium.org/5336005/diff/12001/test/mjsunit/regress/regres...
> test/mjsunit/regress/regress-944.js:31: assertEquals(1290722550521,
> Date.parse("2010-11-25T22:02:30.521Z"));
> Perhaps add some spacing here, before the next comment, purely for readability
> (ditto for the following comments).

OK, have added some newlines.

>
http://codereview.chromium.org/5336005/diff/12001/test/mjsunit/regress/regres...
> test/mjsunit/regress/regress-944.js:38:
> assertEquals(Date.parse("2010-11-25T22:02:30.1006Z"),
> Date.parse("2010-11-25T22:02:30.100Z"));
> Should this be ".1005Z" (and not ".1006Z")?

Yeah, have changed it to that.

[Lasse Reichstein, 2010-11-26 08:01:28]

This looks very good now.
But it just dawns on me that I can't see where the function is called. Is
dateparser-inl.h missing from the CL?
If I download the patch and compile, the result has the old behavior - and the
tests work! (I.e., they fail).

http://codereview.chromium.org/5336005/diff/20001/src/dateparser.h
File src/dateparser.h (right):

http://codereview.chromium.org/5336005/diff/20001/src/dateparser.h#newcode95
src/dateparser.h:95: int p;
Call it something readable, like "power" or "position_value" or what it is "p"
stands for.

http://codereview.chromium.org/5336005/diff/20001/src/dateparser.h#newcode96
src/dateparser.h:96: for (p = 100; IsAsciiDigit(); Next(), p = p/10) {
Put spaces around the division operator.

[sideshowbarker, 2010-11-26 10:54:17]

On 2010/11/26 08:01:28, Lasse Reichstein wrote:
> This looks very good now.
> But it just dawns on me that I can't see where the function is called. Is
> dateparser-inl.h missing from the CL?

Yeah, sorry. I don't know why gcl isn't noticing that it's changed. I guess I'll
try to add it manually.

> http://codereview.chromium.org/5336005/diff/20001/src/dateparser.h
> File src/dateparser.h (right):
> 
> http://codereview.chromium.org/5336005/diff/20001/src/dateparser.h#newcode95
> src/dateparser.h:95: int p;
> Call it something readable, like "power" or "position_value" or what it is "p"
> stands for.

OK, changed it to "power".

> http://codereview.chromium.org/5336005/diff/20001/src/dateparser.h#newcode96
> src/dateparser.h:96: for (p = 100; IsAsciiDigit(); Next(), p = p/10) {
> Put spaces around the division operator.

OK

I will upload the updated patch as soon as I figure out how to get the
dateparser-inl.h diff to be included.

[sideshowbarker, 2010-11-26 11:05:49]

On 2010/11/26 10:54:17, sideshowbarker wrote:
> On 2010/11/26 08:01:28, Lasse Reichstein wrote:
> > This looks very good now.
> > But it just dawns on me that I can't see where the function is called. Is
> > dateparser-inl.h missing from the CL?
> 
> Yeah, sorry. I don't know why gcl isn't noticing that it's changed. I guess
I'll
> try to add it manually.
 
Added now. gcl was not picking it up because it was already in another
changelist I have in my workspace for a previous patch I submitted, and I guess
that means gcl won't let you have a file in more than one uncommitted changelist
at the same time.

[Lasse Reichstein, 2010-11-26 11:48:02]

LGTM! Thank you for the patch. I'll land it on bleeding edge for you.

[sideshowbarker, 2010-11-26 11:51:40]

On 2010/11/26 11:48:02, Lasse Reichstein wrote:
> LGTM! Thank you for the patch. I'll land it on bleeding edge for you.

Sweet :) Thanks very much for you guidance and patience, Lasse.

  --Mike