[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