[webkit-reviews] review granted: [Bug 107503] Fix DateMath.cpp to compile with -Wshorten-64-to-32 : [Attachment 183996] Patch 2 of 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 107503: Fix DateMath.cpp to compile with -Wshorten-64-to-32
https://bugs.webkit.org/show_bug.cgi?id=107503

Attachment 183996: Patch 2 of 2
https://bugs.webkit.org/attachment.cgi?id=183996&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Your changes are OK.

There’s a lot that is wrong about this parse function for WebKit. It’s
basically a cover for the strtol library function:

- The workaround for Windows CE behavior is done here, but in WebKit such
workarounds are supposed to go into <wtf/MathExtras.h>
- The function can’t parse the minimum or maximum values of a given type, an
unconventional limitation that I assume is not a problem in the date parsing
code.
- The function uses the strtol library function. But we have good numeric
parsing functions elsewhere in WTF, which probably didn’t exist when this was
first written. The space skipping behavior of strtol is based on isspace and
thus is affected by the POSIX locale set in the standard library, which is why
we don’t use these kinds of library functions elsewhere in WebKit, and also why
our ASCIICType.h header exists.
- The function retains the strtol “base” argument, even though callers always
pass 10.
- The function uses pointers for its out arguments. While some of the Chromium
project contributors have argued for this, it is not the usual WebKit coding
style. We use references for such things.

Because of that, if I was fixing this, I’d probably also rewrite this to not
use strtol.