Fix issue 5275: The VM must always generate the most compact form of an integer (Smi, Mint or Bigin…

Fix issue 5275: The VM must always generate the most compact form of an integer (Smi, Mint or Bigint). Hide methods that can be used to bypass that assumption and require the use of Integer::NewXXX which is guaranteed to return the most compact integer form.

R=asiva@google.com

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

[siva, 2013-05-16 19:52:50]

LGTM with couple of comments.

https://codereview.chromium.org/14962008/diff/1/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/14962008/diff/1/runtime/lib/double.cc#newcode84
runtime/lib/double.cc:84: return BigintOperations::ToSmi(big);
Do we still need this check?

We could just do:
If (BigintOperations::FitsIntoMint(big)) {
  return Integer::New(BigintOperations::ToMint(big));
}

https://codereview.chromium.org/14962008/diff/1/runtime/lib/typed_data.cc
File runtime/lib/typed_data.cc (right):

https://codereview.chromium.org/14962008/diff/1/runtime/lib/typed_data.cc#new...
runtime/lib/typed_data.cc:261: } else if (value >
static_cast<uint64_t>(Smi::kMaxValue)) {                  \
Ditto comment here.

This could just be:
} else {
  result = Integer::New(value);
}

https://codereview.chromium.org/14962008/diff/1/runtime/lib/typed_data.cc#new...
runtime/lib/typed_data.cc:413: return Integer::New(value);
Ditto here too.
it could just be:

return Integer::New(value);

[srdjan, 2013-05-16 20:03:31]

Thanks.

https://codereview.chromium.org/14962008/diff/1/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/14962008/diff/1/runtime/lib/double.cc#newcode84
runtime/lib/double.cc:84: return BigintOperations::ToSmi(big);
On 2013/05/16 19:52:50, siva wrote:
> Do we still need this check?
> 
> We could just do:
> If (BigintOperations::FitsIntoMint(big)) {
>   return Integer::New(BigintOperations::ToMint(big));
> }

Done.

https://codereview.chromium.org/14962008/diff/1/runtime/lib/typed_data.cc
File runtime/lib/typed_data.cc (right):

https://codereview.chromium.org/14962008/diff/1/runtime/lib/typed_data.cc#new...
runtime/lib/typed_data.cc:261: } else if (value >
static_cast<uint64_t>(Smi::kMaxValue)) {                  \
On 2013/05/16 19:52:50, siva wrote:
> Ditto comment here.
> 
> This could just be:
> } else {
>   result = Integer::New(value);
> }

Done.

https://codereview.chromium.org/14962008/diff/1/runtime/lib/typed_data.cc#new...
runtime/lib/typed_data.cc:413: return Integer::New(value);
On 2013/05/16 19:52:50, siva wrote:
> Ditto here too.
> it could just be:
> 
> return Integer::New(value);

Done.

[srdjan, 2013-05-16 20:34:33]

Also added a fix to BigintOperations::ToMint; it coudl not handle zero Bigints.

[srdjan, 2013-05-16 20:49:13]

Committed patchset #3 manually as r22816 (presubmit successful).

[Ivan Posva, 2013-05-16 21:46:53]

DBC -ip

https://codereview.chromium.org/14962008/diff/16001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/14962008/diff/16001/runtime/lib/double.cc#new...
runtime/lib/double.cc:83: if (BigintOperations::FitsIntoMint(big)) {
I find this very confusing. This would be much more readable and easier to
follow if we changed this to:
if (BigintOperations::FitsIntoInt64(big)) {
  return Integer::New(BigintOperations::ToInt64(big));
}

https://codereview.chromium.org/14962008/diff/16001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14962008/diff/16001/runtime/vm/object.h#newco...
runtime/vm/object.h:4098: RawBigint* ArithmeticOp(Token::Kind operation, const
Bigint& other) const;
Shouldn't this return a RawInteger*?

[srdjan, 2013-05-16 21:53:46]

Thanks!

https://codereview.chromium.org/14962008/diff/16001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/14962008/diff/16001/runtime/lib/double.cc#new...
runtime/lib/double.cc:83: if (BigintOperations::FitsIntoMint(big)) {
On 2013/05/16 21:46:53, Ivan Posva wrote:
> I find this very confusing. This would be much more readable and easier to
> follow if we changed this to:
> if (BigintOperations::FitsIntoInt64(big)) {
>   return Integer::New(BigintOperations::ToInt64(big));
> }

Done in next CL.

https://codereview.chromium.org/14962008/diff/16001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14962008/diff/16001/runtime/vm/object.h#newco...
runtime/vm/object.h:4098: RawBigint* ArithmeticOp(Token::Kind operation, const
Bigint& other) const;
On 2013/05/16 21:46:53, Ivan Posva wrote:
> Shouldn't this return a RawInteger*?

Leaving it as Bigint, but rename and hide. Next CL.