[webkit-reviews] review denied: [Bug 9697] parseInt results may be inaccurate for numbers greater than 2^53 : [Attachment 15528] Revised proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>'s request for review:
Bug 9697: parseInt results may be inaccurate for numbers greater than 2^53
http://bugs.webkit.org/show_bug.cgi?id=9697

Attachment 15528: Revised proposed patch
http://bugs.webkit.org/attachment.cgi?id=15528&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looking great! I love that this fixes so many things in existing tests. I still
see a few problems.

The variable "p" should be declared as a const char* -- then you wouldn't need
the typecast on this line:

+    for (p = ((char*) s) + length - 1; p >= s; p--) {

I don't see any use of p that would require a non-const pointer.

I know Adam suggested this:

+		 s[p] = '\0';

but it won't work -- you can't modify a const UString&. In fact, if it
compiles, we should probably fix this line from the ustring.h header:

    UChar operator[](int pos) const;

If we change it to return a const UChar then you'd see a compiler error.

The right want to do this is simply to call s.substr(firstDigitPosition, p -
firstDigitPosition).ascii(). The UString class has an appropriate optimization,
so this won't have to allocate an entirely new string buffer.

In fact, the other callers should also call
s.substr(firstDigitPosition).ascii(), so the buffer created by the ascii()
function can be smaller.

Are you sure that HUGE_VAL works on all the platforms we need to compile on? I
ask because we have KJS::Inf in value.h and we wouldn't need it if we could
just use HUGE_VAL.

+	   if (strncasecmp(c, "inf", 3) == 0)
+	     return NaN;

I don't understand the code above at all, and I'd like to see the test cases
that demonstrate we need it and why it's OK to just look at the first three
characters.

Because this code was written incorrectly but the tests still passed, I think
we need to devise at least one more test to ensure that everything is working
correctly. If you can't find a test that fails due to the s[p] mistake, then I
suspect that you don't need to do the truncation at all -- I suspect kjs_strtod
will stop just fine when it encounters an "e" or a "." and if not we need a
test to demonstrate it working. In general we need a test that exercises every
branch in the new code, and that's not true at the moment.