[webkit-reviews] review denied: [Bug 44632] Date.parse not ES5 compliant, cannot parse standard Date Time String format : [Attachment 70673] Implemented ECMAScript/RFC3339 date parser in JavaScriptCore

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 70673: Implemented ECMAScript/RFC3339 date parser in JavaScriptCore
https://bugs.webkit.org/attachment.cgi?id=70673&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70673&action=review

review- because of the decimal separator issue

> JavaScriptCore/wtf/DateMath.cpp:566
> +    static unsigned int daysPerMonth[] = {31, 29, 31, 30, 31, 30, 31, 31,
30, 31, 30, 31};

We normally use “unsigned”, not “unsigned int”.

> JavaScriptCore/wtf/DateMath.cpp:569
> +    unsigned int month = 0, day = 0, hours = 0, minutes = 0;

We normally use “unsigned”, not “unsigned int”. We don’t declare multiple
variables on a single line.

> JavaScriptCore/wtf/DateMath.cpp:574
> +    // NOTE: this is a bit more lenient on the year string than ES5
specifies:
> +    //	instead of restricting to 4 digits (or 6 digits with mandatory
+/-),
> +    //	it accepts any integer value. Consider this an implementation
fallback.

We use sentence format for comments, meaning the first word should be
capitalized.

We don’t put NOTE in front of comments.

We don’t indent subsequent lines of comments in this fashion.

> JavaScriptCore/wtf/DateMath.cpp:576
> +    int numFieldsParsed = sscanf(dateString, "%d-%2u-%2u T %2u:%2u:%lf %6s",

> +				    &year, &month, &day, &hours, &minutes,
&seconds, timeZoneBuffer);

We don’t indent subsequent lines in this fashion.

Typically sscanf has poor performance. Would you be willing to write the
parsing without using that function?

The sscanf function will determine which decimal point character based on the
POSIX locale, and I presume we do not want that behavior here, so the seconds
parsing is a bit of a problem.