Please add more comments to bignum-dtoa explaining the basic principles, and
giving a big-picture view of what each function accomplishes. Otherwise,
LGTM.
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.
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.
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
What is wiggle_plus?
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
Can the wiggle stuff be put where wiggle is used?
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode99
src/bignum-dtoa.cc:99: if (need_wiggles) {
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.
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
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.
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode319
src/bignum-dtoa.cc:319: static void GenerateDigits(Bignum* numerator,
Bignum* denominator,
GenerateShortestDigits?
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode423
src/bignum-dtoa.cc:423: // digit = numerator / denominator (integer
division).
Include "Assumes numerator / denominator < 10" (or < 16, or whatever).
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).
Propagate the carry until we hit a non-'9' or til we reach the first
digit.
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode442
src/bignum-dtoa.cc:442: if (buffer[0] == '0' + 10) {
Propagate a carry past the top place.
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode459
src/bignum-dtoa.cc:459: if (-(*decimal_point) > requested_digits) {
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.
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.
Why does this bring the fraction to that range?
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.cc#newcode496
src/bignum-dtoa.cc:496: bool need_wiggles = (mode ==
BIGNUM_DTOA_SHORTEST);
bool compute_shortest_approximation =, instead of need_wiggles?
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.
fewer than
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
fewer
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
Better description, not just an example.
Or say "see below, at declaration of BignumDtoa."
http://codereview.chromium.org/3468003/diff/50001/src/bignum-dtoa.h#newcode37
src/bignum-dtoa.h:37: // Fixed number of digits after the comma.
Return a fixed number of digits after the decimal point (or radix
point).
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).
Return a fixed number of digits, no matter what the exponent is.
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
decimal point, radix point.
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
fewer
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
No memory management functions are passed in anymore.
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
+ 1. Otherwise, the size of the output is limited to requested_digits
digits plus the null terminator.
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.
// Can return the empty string if all digits are 0.
http://codereview.chromium.org/3468003/diff/50001/test/cctest/test-bignum-dtoa.cc#newcode255
test/cctest/test-bignum-dtoa.cc:255:
Are these tests taken from the Gay strtod implementation? Otherwise,
the above tests could be merged into the below ones, right?
http://codereview.chromium.org/3468003/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
