Summary: | Fix DateMath.cpp to compile with -Wshorten-64-to-32 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | Web Template Framework | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | NEW --- | ||||||||
Severity: | Normal | CC: | benjamin, ojan.autocc, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 107093 | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2013-01-21 22:46:16 PST
Created attachment 183893 [details]
Patch
Created attachment 183996 [details]
Patch 2 of 2
(In reply to comment #1) > Created an attachment (id=183893) [details] > Patch The general idea here is to switch 'offset' and 'year' to be type 'int', and continue using 'long' for other time values. Patch 2 of 2 is cleanup to merge parseInt() and parseLong() into a single template function. Comment on attachment 183996 [details]
Patch 2 of 2
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.
Comment on attachment 183893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183893&action=review Seems OK. Things are still a bit of a mess. If we had more time to clean up further, I think we could stop using long here. There’s no reason we need to use strtol when we have perfectly good numeric conversion functions of our own. > Source/WTF/wtf/DateMath.cpp:837 > + if (day <= std::numeric_limits<int>::min() || day >= std::numeric_limits<int>::max()) These should be < and > rather than <= or >=. Unless there is some reason the limit values are a problem. Except that probably will cause warnings on platforms where long and int are the same size. Ugh. Comment on attachment 183893 [details] Patch Clearing flags on attachment: 183893 Committed r140437: <http://trac.webkit.org/changeset/140437> Comment on attachment 183996 [details]
Patch 2 of 2
I'm going to file some follow-up bugs based on Darin's comments rather than landing this patch.
|