http://codereview.chromium.org/4060001/diff/17001/18002 File src/bignum.cc (right): http://codereview.chromium.org/4060001/diff/17001/18002#newcode164 src/bignum.cc:164: void Bignum::AddUInt16(uint16_t operand) { Will you optimize this, either by making a special implementation, or by making "Zero()" zero all values only on debug, and only zero the least significant chunk otherwise? http://codereview.chromium.org/4060001/diff/17001/18002#newcode191 src/bignum.cc:191: int offset = other.exponent_ - exponent_; ASSERT(offset >= 0); ? OK, I see that Align(other) does that. Comment on this. ASSERT(bigit_pos + other.used_digits < kBigitLength? < used_digits? http://codereview.chromium.org/4060001/diff/17001/18002#newcode224 src/bignum.cc:224: Chunk difference = bigits_[i + offset] - other.bigits_[i] - borrow; Note that borrow will always be 0 or 1, since difference >> (kChunkSize -1) is the high bit (sign bit) of an unsigned number, logical right shifted to the LSB. Otherwise, this seems wrong. If you used a signed difference, couldn't you make it parallel to AddBignum? http://codereview.chromium.org/4060001/diff/17001/18002#newcode280 src/bignum.cc:280: uint64_t tmp = (carry & kBigitMask) + product; Use product again instead of tmp, as in MultiplyUInt16. If you make a temp factor64 here, then why not do the same in MultiplyUInt16? Or else just cast factor here as well. http://codereview.chromium.org/4060001/diff/17001/18002#newcode412 src/bignum.cc:412: while (bigit_index2 < used_digits_) { Loop runs 0 times on last iteration, emptying accumulator. http://codereview.chromium.org/4060001/diff/17001/18002#newcode419 src/bignum.cc:419: bigits_[i] = static_cast<Chunk>(accumulator) & kBigitMask; Comment: The overwritten bigits_[i] will never be read in further loop iterations, because bigit_index1 and bigit_index2 are always greater than i - used_digits_. http://codereview.chromium.org/4060001/diff/17001/18002#newcode524 src/bignum.cc:524: while (BigitLength() > other.BigitLength()) { It seems to me this loop could run about 30,000 times. this = 2, 0, 0, 0 other = 0, 2^17 + 1, 0, 0 Instead, take other[used_digits_] and other[used_digits_ -1] and form a DoubleChunk with them? http://codereview.chromium.org/4060001/diff/17001/18002#newcode548 src/bignum.cc:548: int division_estimate = this_bigit / (other_bigit + 1); If other_bigit = 1, then we may have a terrible division_estimate, and only reduce this by about 1/2 each iteration. Again, use a doublechunk, perhaps. You need a minimum # of digits of accuracy in your divisor estimate. http://codereview.chromium.org/4060001/diff/17001/18002#newcode553 src/bignum.cc:553: // No need to even try to subtract. Even if other's remaining digits were 0 I think you can leave this to the next iteration if you do double division each time. http://codereview.chromium.org/4060001/diff/17001/18002#newcode568 src/bignum.cc:568: int result = 0; This will be an infinite loop with a signed type, and a negative number. Comment? ASSERT( number >>4 < number) ? http://codereview.chromium.org/4060001/diff/17001/18002#newcode674 src/bignum.cc:674: return +1; How about: else { borrow = chunk_c + borrow - sum; if borrow > 1 return -1 borrow <<= kBigitSize; } http://codereview.chromium.org/4060001/diff/17001/18002#newcode765 src/bignum.cc:765: DoubleChunk remove = borrow + product; A single DoubleChunk variable can be used for both borrow and remove - their livetimes are disjoint. http://codereview.chromium.org/4060001/diff/17001/18003 File src/bignum.h (right): http://codereview.chromium.org/4060001/diff/17001/18003#newcode61 src/bignum.h:61: void MultiplyUInt64(uint64_t value); Why not "factor"? Should the names say "MultiplyBy"? http://codereview.chromium.org/4060001/diff/17001/18003#newcode69 src/bignum.h:69: bool HexString(char* buffer, int buffer_size) const; Is this ToHexString? Or AssignHexString? http://codereview.chromium.org/4060001/diff/17001/18003#newcode122 src/bignum.h:122: int BigitLength() const { return used_digits_ + exponent_; } Having a variable BigitLength() and a constant kBigitLength seems confusing. Should it be kMaxBigitLength instead? http://codereview.chromium.org/4060001/diff/17001/18003#newcode127 src/bignum.h:127: Vector<Chunk> bigits_; Add comment "A length-kBigitSize vector backed by bigits_buffer_." http://codereview.chromium.org/4060001/diff/17001/18003#newcode129 src/bignum.h:129: int exponent_; // Encodes "hidden" Bigits. What does this really mean. Why not just say BigNum represents value(bigits_) * 2^exponent_. http://codereview.chromium.org/4060001/diff/17001/18009 File tools/gyp/v8.gyp (right): http://codereview.chromium.org/4060001/diff/17001/18009#newcode284 tools/gyp/v8.gyp:284: '../../src/bignum.h', Could you change the Visual Studio project files as well? Are the Mac Xcode files automatically fixed? http://codereview.chromium.org/4060001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
Reviewed all but strtod.cc and the three test files. Their reviews will
follow.
- [v8-dev] Re: Bignum implementation of Strtod. (issue4060001) whesse
