[Webkit-unassigned] [Bug 9697] parseInt results may be inaccurate for numbers greater than 2^53

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=9697


mjs at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #15527|review?                     |review-
               Flag|                            |




------- Comment #9 from mjs at apple.com  2007-07-15 23:48 PDT -------
(From update of attachment 15527)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.