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) { On 2010/05/04 09:43:46, William Hesse wrote:
I think "by" is a bad name. How about: short_factor short_multiplicand multiplicand r r32 factor32
Done. http://codereview.chromium.org/1865001/diff/1/17001#newcode63 src/fixed-dtoa.cc:63: ASSERT(-64 <= by && by <= 64); On 2010/05/04 09:43:46, William Hesse wrote:
shift_amount
Done. http://codereview.chromium.org/1865001/diff/1/17001#newcode84 src/fixed-dtoa.cc:84: // The returned value equals this / 2^position On 2010/05/04 09:43:46, William Hesse wrote:
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.
Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode8 src/fixed-dtoa.cc:8: // * Redistributions in binary form must reproduce the above On 2010/05/04 09:43:46, William Hesse wrote:
The code review tool is showing lint errors on many of the files.
These should
be stamped out.
Many of them are incorrect (for instance we don't have directories for sources). 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) { On 2010/05/04 09:43:46, William Hesse wrote:
A comment or an assert that buffer length is > 9 or 10?
I added a comment in fixed-dtoa.h that requires the buffer to be big enough to hold the result. We continuously check out-of-bounds due to the use of a Vector. I don't see why another assert/comment would help here. 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) { On 2010/05/04 09:43:46, William Hesse wrote:
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.
Two variables is not for efficiency, but I found it easier to read. If you disagree I would be willing to change. Pointer variables are usually slower than accesses using indices (at least in optimized code). http://codereview.chromium.org/1865001/diff/36001/51001#newcode205 src/fixed-dtoa.cc:205: buffer[0] = '1'; On 2010/05/04 09:43:46, William Hesse wrote:
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.
Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode214 src/fixed-dtoa.cc:214: ASSERT(exponent <= 0); On 2010/05/04 09:43:46, William Hesse wrote:
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.
Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode217 src/fixed-dtoa.cc:217: ASSERT(fractionals >> 56 == 0); On 2010/05/04 09:43:46, William Hesse wrote:
Isn't (fractionals >> -exponent == 0) also a precondition?
Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode218 src/fixed-dtoa.cc:218: int point = 0; On 2010/05/04 09:43:46, William Hesse wrote:
Is there a reason to separate the declaration and initialization of
point? Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode224 src/fixed-dtoa.cc:224: fractionals *= 5; On 2010/05/04 09:43:46, William Hesse wrote:
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.
Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode230 src/fixed-dtoa.cc:230: fractionals &= point_mask; On 2010/05/04 09:43:46, William Hesse wrote:
Couldn't point_mask be replaced by fractionals -= static_cast<uint64_t>(digit) << point; ?
much better. thanks. Done. http://codereview.chromium.org/1865001/diff/36001/51001#newcode244 src/fixed-dtoa.cc:244: // point loccation. On 2010/05/04 10:43:15, William Hesse wrote:
location.
Again, comment that point is decremented by 3 before we get numbers
above 1.0 in
fixed point notation, so that we don't overflow.
done
Why not multiply by 5^8, so that you get an int32 worth of integer digits to convert at a time?
I'm not sure it will buy us much. Especially on ARM where there exists no division operation. http://codereview.chromium.org/1865001/diff/36001/51001#newcode286 src/fixed-dtoa.cc:286: if (exponent > 20) return false; On 2010/05/04 10:43:15, William Hesse wrote:
Isn't exponent a binary exponent? I thought 20 was a limit on the
decimal
exponent. Why do we need this (rather low) limit on the binary
exponent? added comment. http://codereview.chromium.org/1865001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
