Option to truncate integers to 64 bits, part 2

Option to truncate integers to 64 bits, part 2

In --limit-ints-to-64-bits mode:
* Integer constructors return Integer::null if integer is out of range.
* Error is reported for integer literals which are out of range.
* Dart API is revised to return errors if integers are out of range.

Bigint::IsDisabled() method is introduced to be able to enable/disable
Bigints independently of --limit-ints-to-64-bits mode in future.

Deprecated constructor Integer::NewFromUint64 is replaced with
Integer::New in certain cases.

R=zra@google.com

Issue: https://github.com/dart-lang/sdk/issues/30103
Committed: https://github.com/dart-lang/sdk/commit/abed3c2a70856d54615890596a4a5db6cffe124f

[alexmarkov, 2017-07-14 14:54:52]

alexmarkov@google.com changed reviewers:
+ asiva@google.com, regis@google.com, zra@google.com

[zra, 2017-07-14 17:36:31]

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:3102: // TODO(alexmarkov): Figure out if Uint64
should be rejected in
I think we'll probably need to keep these. When we pull them out of the array,
we can either complain or make a BigInt depending on whether their enabled.

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:3272: // TODO(alexmarkov): Figure out if Uint64
should be rejected in
ditto

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8507: if (descriptor.kind == Token::kINTEGER) {
Maybe ASSERT this instead of if {} else { UNREACHABLE(); }

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8512: Thread::Current(), "integer literal is out of
range"));
Can you add the value to the error message from *descriptor.literal?

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object_test....
runtime/vm/object_test.cc:656: EXPECT(FLAG_limit_ints_to_64_bits ? i.IsNull() :
i.IsBigint());
Use BigInt::IsDisabled()?

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object_test....
runtime/vm/object_test.cc:659: EXPECT(FLAG_limit_ints_to_64_bits ? i.IsNull() :
i.IsBigint());
ditto.

[alexmarkov, 2017-07-14 18:23:46]

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:3102: // TODO(alexmarkov): Figure out if Uint64
should be rejected in
On 2017/07/14 17:36:30, zra wrote:
> I think we'll probably need to keep these. When we pull them out of the array,
> we can either complain or make a BigInt depending on whether their enabled.

Done.

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:3272: // TODO(alexmarkov): Figure out if Uint64
should be rejected in
On 2017/07/14 17:36:30, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8507: if (descriptor.kind == Token::kINTEGER) {
On 2017/07/14 17:36:30, zra wrote:
> Maybe ASSERT this instead of if {} else { UNREACHABLE(); }

Done.

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8512: Thread::Current(), "integer literal is out of
range"));
On 2017/07/14 17:36:30, zra wrote:
> Can you add the value to the error message from *descriptor.literal?

Done.

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object_test....
runtime/vm/object_test.cc:656: EXPECT(FLAG_limit_ints_to_64_bits ? i.IsNull() :
i.IsBigint());
On 2017/07/14 17:36:30, zra wrote:
> Use BigInt::IsDisabled()?

I think FLAG_limit_ints_to_64_bits is more appropriate here. This flag controls
whether the implicit creation of Bigints is performed (including inside Integer
constructors). Bigint::IsDisabled() tells whether the Bigint class can be used
at all and more appropriate to check when Bigint is used explicitly.

https://codereview.chromium.org/2982823002/diff/20001/runtime/vm/object_test....
runtime/vm/object_test.cc:659: EXPECT(FLAG_limit_ints_to_64_bits ? i.IsNull() :
i.IsBigint());
On 2017/07/14 17:36:30, zra wrote:
> ditto.

Ditto :)

[zra, 2017-07-14 20:20:17]

lgtm

[alexmarkov, 2017-07-14 20:24:57]

Description was changed from

==========
Option to truncate integers to 64 bits, part 2

In --limit-ints-to-64-bits mode:
* Integer constructors return Integer::null if integer is out of range.
* Error is reported for integer literals which are out of range.
* Dart API is revised to return errors if integers are out of range.

Bigint::IsDisabled() method is introduced to be able to enable/disable
Bigints independently of --limit-ints-to-64-bits mode in future.

Deprecated constructor Integer::NewFromUint64 is replaced with
Integer::New in certain cases.

Issue: https://github.com/dart-lang/sdk/issues/30103
==========

to

==========
Option to truncate integers to 64 bits, part 2

In --limit-ints-to-64-bits mode:
* Integer constructors return Integer::null if integer is out of range.
* Error is reported for integer literals which are out of range.
* Dart API is revised to return errors if integers are out of range.

Bigint::IsDisabled() method is introduced to be able to enable/disable
Bigints independently of --limit-ints-to-64-bits mode in future.

Deprecated constructor Integer::NewFromUint64 is replaced with
Integer::New in certain cases.

R=zra@google.com

Issue: https://github.com/dart-lang/sdk/issues/30103
Committed:
https://github.com/dart-lang/sdk/commit/abed3c2a70856d54615890596a4a5db6cffe124f
==========

[alexmarkov, 2017-07-14 20:24:59]

Committed patchset #3 (id:40001) manually as
abed3c2a70856d54615890596a4a5db6cffe124f (presubmit successful).

[siva, 2017-07-14 20:53:29]

https://codereview.chromium.org/2982823002/diff/40001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/2982823002/diff/40001/runtime/lib/integers.cc...
runtime/lib/integers.cc:33: }
When --limit-ints-to-64-bits mode is specified i.IsBigint() should always be
false right.

I am wondering if we will be eliminating some code from the VM if we used
if (!FLAG_limit_ints_to_64_bits) {
} else {
  ....
}
when the flag becomes a const with a value of true.

In other words I am asking if there is a plan in future CLs to completely remove
the BigInt stuff from the executable when the flag FLAG_limit_ints_to_64_bits is
specified.

[zra, 2017-07-14 21:04:18]

On 2017/07/14 20:53:29, siva wrote:
> https://codereview.chromium.org/2982823002/diff/40001/runtime/lib/integers.cc
> File runtime/lib/integers.cc (right):
> 
>
https://codereview.chromium.org/2982823002/diff/40001/runtime/lib/integers.cc...
> runtime/lib/integers.cc:33: }
> When --limit-ints-to-64-bits mode is specified i.IsBigint() should always be
> false right.
> 
> I am wondering if we will be eliminating some code from the VM if we used
> if (!FLAG_limit_ints_to_64_bits) {
> } else {
>   ....
> }
> when the flag becomes a const with a value of true.
> 
> In other words I am asking if there is a plan in future CLs to completely
remove
> the BigInt stuff from the executable when the flag FLAG_limit_ints_to_64_bits
is
> specified.

A way to completely remove BigInt stuff probably isn't needed since we'll need
to have it around when BigInts are part of the public API separate from 'int'