[webkit-reviews] review granted: [Bug 44632] Date.parse not ES5 compliant, cannot parse standard Date Time String format : [Attachment 70798] Rewrote patch based on feedback

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nathan Vander Wilt
<natevw at yahoo.com>'s request for review:
Bug 44632: Date.parse not ES5 compliant, cannot parse standard Date Time String
format
https://bugs.webkit.org/show_bug.cgi?id=44632

Attachment 70798: Rewrote patch based on feedback
https://bugs.webkit.org/attachment.cgi?id=70798&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.