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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jul 15 23:48:10 PDT 2007


Maciej Stachowiak <mjs 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 15527: Proposed patch
http://bugs.webkit.org/attachment.cgi?id=15527&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
The substance of this fix looks great! However, I have a number of coding style
comment.s

1) You added an awful lot of code inline to parseInt, UString::toDouble and the
lexer. I would suggest factoring this out into separate functions. If you name
it properly you may need less in the way of comments, and it may make it easier
to refactor the code later to share more code.

2) 9007199254740992.0 is a mysterious magic number to have in the code. I would
suggest using a named constant (static const double). If you name it well, you
might not need quite so much in the way of comments.

3) Too much comment volume. We usually stick to comments that explain *why* the
code is doing something, not *what* it is doing. The code should be written so
that *what* it is doing is clear, through appropriate use of variable names and
through factoring things out into functions. An explanation can be made for
particularly subtle algorithms. Probably the most helpful comment would be that
the "different method to ensure accuracy" is to calculate starting from the
least significant digit, instead of from the most significant digit.

4) You say "If the radix is a power of 2, we can use a simpler algorithm that
works because the radix has an exact floating-point representation". But I
don't see any special casing for the radix being a power of two. Did I miss
something?

5) It seems that numbers really have to be all-ASCII, perhaps it would be
possible to share more code by coding the slower large number fallback in
ASCII-only terms, and convert when it is needed, since it is an unusual case
and already going to be slower.

6) (*c & 0xdf) -- this is a tricky way to do case folding, maybe such
trickiness is not needed on the slow fallback path and you could just use
toupper.

7) Perhaps better to use strncasecmp here than to optimize this rare case.
+	   if ((c[0] == 'I' || c[0] == 'i') && (c[1] == 'N' || c[1] == 'n') &&
(c[2] == 'F' || c[2] == 'f'))

r- based on the multiplicity of style comments, but I'd love to have this patch
in once they are addressed. Fixing those JS tests is good stuff.



More information about the webkit-reviews mailing list