Reviewed fixed-dtoa.cc to line 235. Almost done.
http://codereview.chromium.org/1865001/diff/1/17001 File src/fixed-dtoa.cc (right): http://codereview.chromium.org/1865001/diff/1/17001#newcode45 src/fixed-dtoa.cc:45: void Multiply(uint32_t by) { I think "by" is a bad name. How about: short_factor short_multiplicand multiplicand r r32 factor32 http://codereview.chromium.org/1865001/diff/1/17001#newcode63 src/fixed-dtoa.cc:63: ASSERT(-64 <= by && by <= 64); shift_amount http://codereview.chromium.org/1865001/diff/1/17001#newcode84 src/fixed-dtoa.cc:84: // The returned value equals this / 2^position The standard name for this is DivMod, I think. Maybe DivModPowerof2? I would say: Modifies *this to *this MOD (2 ^ position), returns *this DIV (2 ^ position). Or use (1 << position) instead of 2 ^ position. http://codereview.chromium.org/1865001/diff/36001/51001#newcode8 src/fixed-dtoa.cc:8: // * Redistributions in binary form must reproduce the above The code review tool is showing lint errors on many of the files. These should be stamped out. http://codereview.chromium.org/1865001/diff/36001/51001#newcode133 src/fixed-dtoa.cc:133: static void FillDigits32(uint32_t number, Vector<char> buffer, int* length) { A comment or an assert that buffer length is > 9 or 10? http://codereview.chromium.org/1865001/diff/36001/51001#newcode143 src/fixed-dtoa.cc:143: for (int i = 0, j = number_length - 1; i < j; ++i, --j) { If you go to the trouble of making two variables for efficiency, why not make them pointer variables, since all the uses are of the same form? Can you check left < right with two pointers in the same array? Yes: If two pointers point to elements of the same array or to the element one beyond the end of the array, the pointer to the object with the higher subscript compares higher. Comparison of pointers is guaranteed valid only when the pointers refer to objects in the same array or to the location one past the end of the array. http://codereview.chromium.org/1865001/diff/36001/51001#newcode143 src/fixed-dtoa.cc:143: for (int i = 0, j = number_length - 1; i < j; ++i, --j) { char* i = buffer + *length, *j = i + number_length - 1; i < j; ++i, --j http://codereview.chromium.org/1865001/diff/36001/51001#newcode205 src/fixed-dtoa.cc:205: buffer[0] = '1'; This needs a comment. It is only because carries started in the units place that we know all the digits have changed to '0', so that adding an implicit 0 on the right is the same as inserting '10' on the left. http://codereview.chromium.org/1865001/diff/36001/51001#newcode214 src/fixed-dtoa.cc:214: ASSERT(exponent <= 0); Comment: fractionals is a fixed-point number, with binary point at bit (-exponent). Inside the function, the non-converted remainder of fractionals is a fixed point number, with binary point at bit point. http://codereview.chromium.org/1865001/diff/36001/51001#newcode217 src/fixed-dtoa.cc:217: ASSERT(fractionals >> 56 == 0); Isn't (fractionals >> -exponent == 0) also a precondition? http://codereview.chromium.org/1865001/diff/36001/51001#newcode218 src/fixed-dtoa.cc:218: int point = 0; Is there a reason to separate the declaration and initialization of point? http://codereview.chromium.org/1865001/diff/36001/51001#newcode224 src/fixed-dtoa.cc:224: fractionals *= 5; The fact that this multiplication doesn't overflow is subtle. Based on the fact that if exponent is 64, then by the time that fractionals (which starts less that 1 << 56) gets to be 61 bits long, then point is 62, and 61 ofter the decrement. Should be commented. http://codereview.chromium.org/1865001/diff/36001/51001#newcode230 src/fixed-dtoa.cc:230: fractionals &= point_mask; Couldn't point_mask be replaced by fractionals -= static_cast<uint64_t>(digit) << point; ? http://codereview.chromium.org/1865001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
