Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(140)

Issue 5336005: make DateParser::TimeComposer handle milliseconds properly... (Closed)

Created:
10 years ago by sideshowbarker
Modified:
9 years, 7 months ago
Reviewers:
Lasse Reichstein, floitschv8
CC:
v8-dev
Visibility:
Public.

Description

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.

Patch Set 1 : '' #

Total comments: 4

Patch Set 2 : '' #

Total comments: 5

Patch Set 3 : '' #

Total comments: 2

Patch Set 4 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -1 line) Patch
M src/dateparser.h View 1 2 3 1 chunk +12 lines, -0 lines 0 comments Download
M src/dateparser-inl.h View 1 chunk +1 line, -1 line 0 comments Download
A test/mjsunit/regress/regress-944.js View 1 2 1 chunk +46 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
sideshowbarker
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. ...
10 years ago (2010-11-25 09:33:15 UTC) #1
Lasse Reichstein
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) ...
10 years ago (2010-11-25 10:17:18 UTC) #2
sideshowbarker
Thanks for the review. Please see my responses below. On 2010/11/25 10:17:18, Lasse Reichstein wrote: ...
10 years ago (2010-11-25 12:40:51 UTC) #3
Lasse Reichstein
That's a good approach to reading decimals. Fewer loops than my suggestion :). http://codereview.chromium.org/5336005/diff/12001/src/dateparser.h File ...
10 years ago (2010-11-25 12:56:05 UTC) #4
Florian Loitsch
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, ...
10 years ago (2010-11-25 13:55:53 UTC) #5
Lasse Reichstein
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, ...
10 years ago (2010-11-25 14:10:36 UTC) #6
sideshowbarker
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 ...
10 years ago (2010-11-25 22:28:32 UTC) #7
Lasse Reichstein
This looks very good now. But it just dawns on me that I can't see ...
10 years ago (2010-11-26 08:01:28 UTC) #8
sideshowbarker
On 2010/11/26 08:01:28, Lasse Reichstein wrote: > This looks very good now. > But it ...
10 years ago (2010-11-26 10:54:17 UTC) #9
sideshowbarker
On 2010/11/26 10:54:17, sideshowbarker wrote: > On 2010/11/26 08:01:28, Lasse Reichstein wrote: > > This ...
10 years ago (2010-11-26 11:05:49 UTC) #10
Lasse Reichstein
LGTM! Thank you for the patch. I'll land it on bleeding edge for you.
10 years ago (2010-11-26 11:48:02 UTC) #11
sideshowbarker
10 years ago (2010-11-26 11:51:40 UTC) #12
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

Powered by Google App Engine
This is Rietveld 408576698