Implement correct bigint to double conversion.

Implement correct bigint to double conversion.

Committed: https://code.google.com/p/dart/source/detail?r=5863

[cshapiro, 2012-03-07 21:49:17]

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations.cc
File runtime/vm/bigint_operations.cc (right):

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations.cc...
runtime/vm/bigint_operations.cc:401: if (bigint.IsZero()) {
This fast path should be expanded to covert values that are in the signed 64-bit
integer range as it is possible to have bigint objects whose values in the mint
and smi range.

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations.cc...
runtime/vm/bigint_operations.cc:404: 
How about an early exit here if the value is too large to fit into a double?  We
can quickly reject a large class of out-of-range values up front.

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations.cc...
runtime/vm/bigint_operations.cc:439: 
Why not use the ShiftRight method to extract the upper 54 bits?

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations.cc...
runtime/vm/bigint_operations.cc:528: value = -value;
Why not put the sign bit in when you are constructing "value" above?

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations_te...
File runtime/vm/bigint_operations_test.cc (right):

http://codereview.chromium.org/9540015/diff/1/runtime/vm/bigint_operations_te...
runtime/vm/bigint_operations_test.cc:167: bigint =
BigintOperations::NewFromCString(
test mint range values too?

[floitsch, 2012-03-20 04:06:48]

PTAL.

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
File runtime/vm/bigint_operations.cc (right):

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
runtime/vm/bigint_operations.cc:401: if (bigint.IsZero()) {
On 2012/03/07 21:49:18, cshapiro wrote:
> This fast path should be expanded to covert values that are in the signed
64-bit
> integer range as it is possible to have bigint objects whose values in the
mint
> and smi range.

Done.

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
runtime/vm/bigint_operations.cc:404: 
On 2012/03/07 21:49:18, cshapiro wrote:
> How about an early exit here if the value is too large to fit into a double? 
We
> can quickly reject a large class of out-of-range values up front.

Done.

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
runtime/vm/bigint_operations.cc:439: 
On 2012/03/07 21:49:18, cshapiro wrote:
> Why not use the ShiftRight method to extract the upper 54 bits?

We could, but it seems overkill. I hope the new version is simple enough.

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
runtime/vm/bigint_operations.cc:528: value = -value;
On 2012/03/07 21:49:18, cshapiro wrote:
> Why not put the sign bit in when you are constructing "value" above?

It seems cleaner not to duplicate the bit_cast expression. It wouldn't fit on
one line anyways.

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
File runtime/vm/bigint_operations_test.cc (right):

http://chromiumcodereview.appspot.com/9540015/diff/1/runtime/vm/bigint_operat...
runtime/vm/bigint_operations_test.cc:167: bigint =
BigintOperations::NewFromCString(
On 2012/03/07 21:49:18, cshapiro wrote:
> test mint range values too?

Done.

[cshapiro, 2012-03-23 00:44:39]

lgtm

http://chromiumcodereview.appspot.com/9540015/diff/16001/runtime/vm/bigint_op...
File runtime/vm/bigint_operations.cc (right):

http://chromiumcodereview.appspot.com/9540015/diff/16001/runtime/vm/bigint_op...
runtime/vm/bigint_operations.cc:495: double absolute_value =
static_cast<uint64_t>(AbsToUint64(bigint));
Is the static_cast<uint64_t>(...) needed?  AbsToUint64 returns a uint64_t.  Am I
missing something?

http://chromiumcodereview.appspot.com/9540015/diff/16001/runtime/vm/bigint_op...
runtime/vm/bigint_operations.cc:547: uint64_t twice_significand_floor = 0;
Can we fold this into line 552?  There seem to be no intervening uses of
twice_significand_floor and the assignment to firstDigit.

[floitsch, 2012-03-26 22:52:05]

I slightly refactored the code so that twice_significand_floor and
twice_significand_exponent are in sync.
I assume it's still ok to submit, but let you some hours to have another look,
before submitting.

http://chromiumcodereview.appspot.com/9540015/diff/16001/runtime/vm/bigint_op...
File runtime/vm/bigint_operations.cc (right):

http://chromiumcodereview.appspot.com/9540015/diff/16001/runtime/vm/bigint_op...
runtime/vm/bigint_operations.cc:495: double absolute_value =
static_cast<uint64_t>(AbsToUint64(bigint));
On 2012/03/23 00:44:39, cshapiro wrote:
> Is the static_cast<uint64_t>(...) needed?  AbsToUint64 returns a uint64_t.  Am
I
> missing something? 

typo. Should have been static_cast<double>.
done.

http://chromiumcodereview.appspot.com/9540015/diff/16001/runtime/vm/bigint_op...
runtime/vm/bigint_operations.cc:547: uint64_t twice_significand_floor = 0;
On 2012/03/23 00:44:39, cshapiro wrote:
> Can we fold this into line 552?  There seem to be no intervening uses of
> twice_significand_floor and the assignment to firstDigit.

done.