Reviewers: William Hesse, Message: Please have another look.
http://codereview.chromium.org/2000004/diff/19001/14002 File src/fast-dtoa.cc (right): http://codereview.chromium.org/2000004/diff/19001/14002#newcode78 src/fast-dtoa.cc:78: // ]w_low; w_low[ (often written as "(w_low; w_low)") On 2010/07/30 09:34:36, William Hesse wrote:
(w_low, w_high)
Done. http://codereview.chromium.org/2000004/diff/19001/14002#newcode126 src/fast-dtoa.cc:126: // the buffer. If we have two potential representations we need to make sure On 2010/07/30 09:34:36, William Hesse wrote:
when finding the closest representation of 'w'. If we have two
potential
representations, and one is closer to both w_low and w_high, then we
know it is
closer to the actual value v.
Done. http://codereview.chromium.org/2000004/diff/19001/14002#newcode146 src/fast-dtoa.cc:146: buffer[length - 1]--; On 2010/07/30 09:34:36, William Hesse wrote:
Comment that all these tests are necessary in this order to detect
overflow and
underflow of uint64 values. ASSERT(rest <= unsafe_interval).
Done. http://codereview.chromium.org/2000004/diff/19001/14002#newcode169 src/fast-dtoa.cc:169: // Rounds the buffer according to rest for the counted digit generation method. On 2010/07/30 09:34:36, William Hesse wrote:
Rounds the buffer upwards, if the result is closer to v, by possibly
adding 1 to
the buffer. If the precision of the calculation is not sufficient to
round
correctly, return false.
Done. http://codereview.chromium.org/2000004/diff/19001/14002#newcode192 src/fast-dtoa.cc:192: // over/underflow.) On 2010/07/30 09:34:36, William Hesse wrote:
Comment that these tests are designed to avoid overflow, and will work
correctly
for any uint64 values of rest < ten_kappa and unit.
Done. http://codereview.chromium.org/2000004/diff/19001/14002#newcode210 src/fast-dtoa.cc:210: // the power (the kappa) is increased. On 2010/07/30 09:34:36, William Hesse wrote:
This is slightly weird because the calling function will need to
update
ten_kappa, but also needs to update rest with the old value of
ten_kappa. Does
this function have the right interface, or could it be simplified by
doing more
or less?
The caller basically says: here is my result. Round it if needed. And rounding includes adjusting the kappa. So the caller doesn't need to do anything. The callers actually do a tail-call: return RoundWeed(...). But if you think I should change the interface we can discuss this. http://codereview.chromium.org/2000004/diff/19001/14002#newcode232 src/fast-dtoa.cc:232: // Precondition: (1 << number_bits) <= number < (1 << (number_bits + 1)). On 2010/07/30 09:34:36, William Hesse wrote:
Because we always test, in the switch statement, the first inequality
of the
precondition is not actually necessary. Should you avoid the testing in the switch statement (for example, if number_bits = 31, you know number > kTen9)?
Removed first part of precondition. http://codereview.chromium.org/2000004/diff/19001/14002#newcode490 src/fast-dtoa.cc:490: // * w is correct up to 1 ulp (unit in the last place). That On 2010/07/30 13:30:37, William Hesse wrote:
What does this mean? w is the input. Isn't it exact? What is
"they" - w is a
single number.
Do you mean w represents a range of numbers between w - ulp and w +
ulp? bad copy paste from DigitGen. http://codereview.chromium.org/2000004/diff/19001/14002#newcode498 src/fast-dtoa.cc:498: // digit is rounded according to the rest. On 2010/07/30 13:30:37, William Hesse wrote:
According to the rest of what? Why not "is rounded correctly"
Reworded. http://codereview.chromium.org/2000004/diff/19001/14002#newcode500 src/fast-dtoa.cc:500: // On 2010/07/30 13:30:37, William Hesse wrote:
Isn't |eps| < 10^kappa / 2?
good catch. http://codereview.chromium.org/2000004/diff/19001/14002#newcode524 src/fast-dtoa.cc:524: uint32_t divider; On 2010/07/30 13:30:37, William Hesse wrote:
divisor, rather than divider.
Done. http://codereview.chromium.org/2000004/diff/19001/14002#newcode553 src/fast-dtoa.cc:553: kappa); On 2010/07/30 13:30:37, William Hesse wrote:
Why do we need to cut off a trailing 0 and reduce kappa by one in this
case?
Isn't it ok to leave it since we know we want that many digits?
I don't understand your question. At this point we have requested_digits, and simply need to round the number upwards, if the rest is greater than .5. http://codereview.chromium.org/2000004/diff/19001/14002#newcode560 src/fast-dtoa.cc:560: // Instead of multiplying by 10 we multiply by 5 (cheaper operation) and On 2010/07/30 13:30:37, William Hesse wrote:
Is it really a saving to multiply by 5 rather than 10 in this case?
If we
don't, then we don't have to manipulate the exponent.
changed to *10. http://codereview.chromium.org/2000004/diff/19001/25001 File src/fast-dtoa.h (right): http://codereview.chromium.org/2000004/diff/19001/25001#newcode35 src/fast-dtoa.h:35: // 0.9999999999999999 becomes 0.1 On 2010/07/30 09:34:36, William Hesse wrote:
I don't understand this comment.
Done. http://codereview.chromium.org/2000004/diff/19001/25001#newcode59 src/fast-dtoa.h:59: // 0.099999999999 instead of 0.1. On 2010/07/30 09:34:36, William Hesse wrote:
If 0.099999999999 and 0.1 represent the same double value, "1" is
returned with
point = 0.
Done. http://codereview.chromium.org/2000004/diff/19001/25001#newcode65 src/fast-dtoa.h:65: // the difference v - (buffer * 10^(point-length)) is the smallest for all On 2010/07/30 09:34:36, William Hesse wrote:
is the closest to zero has the smallest absolute value
if two values are equally close, we round to an even last digit.
FastDtoa will always return false if two values are equally close. http://codereview.chromium.org/2000004/diff/19001/25001#newcode66 src/fast-dtoa.h:66: // possible decimal numbers of requested_digits digits. On 2010/07/30 09:34:36, William Hesse wrote:
for all decimal integers "buffer" with requested_digits digits.
I don't like quotes though.
Done. http://codereview.chromium.org/2000004/diff/19001/25001#newcode67 src/fast-dtoa.h:67: // For both modes the buffer must be big enough to hold the result. On 2010/07/30 09:34:36, William Hesse wrote:
must be large enough
Done. Description: Added precision mode to fast-dtoa. Please review this at http://codereview.chromium.org/2000004/show SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ Affected files: M src/conversions.cc M src/dtoa.cc M src/fast-dtoa.h M src/fast-dtoa.cc M test/cctest/SConscript A test/cctest/gay-precision.h M test/cctest/test-fast-dtoa.cc -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
