[Webkit-unassigned] [Bug 44632] Date.parse not ES5 compliant, cannot parse standard Date Time String format
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 14 17:06:16 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=44632
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #70798|review? |review+
Flag| |
--- Comment #5 from Darin Adler <darin at apple.com> 2010-10-14 17:06:16 PST ---
(From update of attachment 70798)
View in context: https://bugs.webkit.org/attachment.cgi?id=70798&action=review
I’d like to see still-more test cases. I don’t think these tests cover every branch in the function. I believe I can remove some of the code and still have all the tests pass.
This is OK to land as-is, but I’d prefer to see an even better version of the patch with more tests and more-conventional WebKit coding style.
> JavaScriptCore/wtf/DateMath.cpp:567
> + static unsigned daysPerMonth[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
Should be “static const”. Also would be nice to specify 12 here as the array size. Our normal style is to put spaces inside the braces.
> JavaScriptCore/wtf/DateMath.cpp:577
> + long year;
> + long month;
> + long day;
> + long hours;
> + long minutes;
> + long intSeconds;
> + double seconds;
> +
> + char* postParsePosition;
Would be nicer to define these right where they are initialized rather than defining them all at the top.
> JavaScriptCore/wtf/DateMath.cpp:586
> + dateString = postParsePosition + 1;
It’s a little strange to keep moving this pointer along, but call it “date string”, since it’s a pointer to the remainder of the string, not the entire string. Might be better to give this a different name.
> JavaScriptCore/wtf/DateMath.cpp:628
> + long numFracDigits;
This especially should be defined on the line where it’s initialized.
> JavaScriptCore/wtf/DateMath.cpp:643
> + if (day < 1 || day > daysPerMonth[month-1])
We put spaces around operators like the "-" here.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list