Please have another look.
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc File src/bignum-dtoa.cc (right): http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode39 src/bignum-dtoa.cc:39: // Returns an estimation of k such that 10^(k-1) <= v < 10^k. On 2010/11/15 15:48:30, William Hesse wrote:
This is unclear. Explains that the input is the exponent from a
normalized fp
number f * 2^e, where 2^52 <= f < 2^53. The output is an
approximation for the
exponent of the decimal approximation .digits * 10^k.
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode41 src/bignum-dtoa.cc:41: // Note: the same is true for v + wiggle_plus (instead of simply v) (see On 2010/11/15 15:48:30, William Hesse wrote:
What is wiggle_plus?
Changed to v+ (v's positive boundary). http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode66 src/bignum-dtoa.cc:66: // Explanation for (v + wiggle_plus): the computation takes advantage of the On 2010/11/15 15:48:30, William Hesse wrote:
Can the wiggle stuff be put where wiggle is used?
Reworked comment. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode99 src/bignum-dtoa.cc:99: if (need_wiggles) { On 2010/11/15 15:48:30, William Hesse wrote:
Call these delta_plus and delta_minus, or something, and explain that
v_plus =
(f + delta_plus) * 2^e is the boundary between v and the next
representable
float, and so on.
Refactored (slightly) method and comments. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode312 src/bignum-dtoa.cc:312: // Produces the least amount of digits so that the result lies in the wiggle On 2010/11/15 15:48:30, William Hesse wrote:
Give more big picture comments - result lying in the wiggle room means
produce
enough digits d of strtod(v) so that v is the closest representable
number to d
* 10^exp.
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode319 src/bignum-dtoa.cc:319: static void GenerateDigits(Bignum* numerator, Bignum* denominator, On 2010/11/15 15:48:30, William Hesse wrote:
GenerateShortestDigits?
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode423 src/bignum-dtoa.cc:423: // digit = numerator / denominator (integer division). On 2010/11/15 15:48:30, William Hesse wrote:
Include "Assumes numerator / denominator < 10" (or < 16, or whatever).
added ASSERT. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode436 src/bignum-dtoa.cc:436: // Correct bad digits (in case we had a sequence of '9's). On 2010/11/15 15:48:30, William Hesse wrote:
Propagate the carry until we hit a non-'9' or til we reach the first
digit. Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode442 src/bignum-dtoa.cc:442: if (buffer[0] == '0' + 10) { On 2010/11/15 15:48:30, William Hesse wrote:
Propagate a carry past the top place.
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode459 src/bignum-dtoa.cc:459: if (-(*decimal_point) > requested_digits) { On 2010/11/15 15:48:30, William Hesse wrote:
Explain this. People don't know what all these quantities are. // Number is smaller than 0.00...001, with requested_digits 0s after
the decimal
point. Return 0.
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode465 src/bignum-dtoa.cc:465: denominator->Times10(); // Bring fraction back to range 0.1 - 1. On 2010/11/15 15:48:30, William Hesse wrote:
Why does this bring the fraction to that range?
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode496 src/bignum-dtoa.cc:496: bool need_wiggles = (mode == BIGNUM_DTOA_SHORTEST); On 2010/11/15 15:48:30, William Hesse wrote:
bool compute_shortest_approximation =, instead of need_wiggles?
renamed to need_boundary_deltas. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode517 src/bignum-dtoa.cc:517: // 4e-324. In this case the denominator needs less than 324*4 binary digits. On 2010/11/15 15:48:30, William Hesse wrote:
fewer than
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode518 src/bignum-dtoa.cc:518: // The maximum double is 1.7976931348623157e308 which needs less than On 2010/11/15 15:48:30, William Hesse wrote:
fewer
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h File src/bignum-dtoa.h (right): http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode35 src/bignum-dtoa.h:35: // 0.9999999999999999 becomes 0.1 On 2010/11/15 15:48:30, William Hesse wrote:
Better description, not just an example.
Or say "see below, at declaration of BignumDtoa."
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode37 src/bignum-dtoa.h:37: // Fixed number of digits after the comma. On 2010/11/15 15:48:30, William Hesse wrote:
Return a fixed number of digits after the decimal point (or radix
point). Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode41 src/bignum-dtoa.h:41: // Fixed number of digits (independent of the comma). On 2010/11/15 15:48:30, William Hesse wrote:
Return a fixed number of digits, no matter what the exponent is.
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode59 src/bignum-dtoa.h:59: // 'requested_digits' digits after the comma. The produced digits might be too On 2010/11/15 15:48:30, William Hesse wrote:
decimal point, radix point.
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode69 src/bignum-dtoa.h:69: // 'requested_digits', the function is allowed to return less digits, in which On 2010/11/15 15:48:30, William Hesse wrote:
fewer
Done. http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode72 src/bignum-dtoa.h:72: // The given memory-management functions are only needed for internal bignum On 2010/11/15 15:48:30, William Hesse wrote:
No memory management functions are passed in anymore.
Done. http://codereview.chromium.org/3468003/diff/50001/src/dtoa.h File src/dtoa.h (right): http://codereview.chromium.org/3468003/diff/50001/src/dtoa.h#newcode76 src/dtoa.h:76: // at least kBase10MaximalLength + 1 and otherwise the requested_digits On 2010/11/15 15:48:30, William Hesse wrote:
+ 1. Otherwise, the size of the output is limited to requested_digits
digits
plus the null terminator.
Done. http://codereview.chromium.org/3468003/diff/50001/test/cctest/test-bignum-dtoa.cc File test/cctest/test-bignum-dtoa.cc (right): http://codereview.chromium.org/3468003/diff/50001/test/cctest/test-bignum-dtoa.cc#newcode44 test/cctest/test-bignum-dtoa.cc:44: // Removes trailing '0' digits. On 2010/11/15 15:48:30, William Hesse wrote:
// Can return the empty string if all digits are 0.
Done. http://codereview.chromium.org/3468003/diff/50001/test/cctest/test-bignum-dtoa.cc#newcode255 test/cctest/test-bignum-dtoa.cc:255: On 2010/11/15 15:48:30, William Hesse wrote:
Are these tests taken from the Gay strtod implementation? Otherwise,
the above
tests could be merged into the below ones, right?
They could be merged, but the numbers from Gay are simply random values, whereas most of the "Various" ones try to exercise different parts of the dtoa-routine. http://codereview.chromium.org/3468003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
