Make sure Bigint._digits is always a Uint32List (instead of sometimes null).

Make sure Bigint._digits is always a Uint32List and bever null. Adapt type information for get:_digits.

R=regis@google.com

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

[srdjan, 2014-09-18 18:23:59]

srdjan@google.com changed reviewers:
+ regis@google.com

[regis, 2014-09-18 20:18:22]

You also need to modify bigint creation in api messages.
See dart_api_messages.cc:98

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart#...
runtime/lib/bigint.dart:77: }
If I understand correctly, this indirection only verifies a correct
implementation of _Bigint, but it is not free, unless the optimizer can
eliminate it. I would remove it either now or when we declare the fields in
Dart.

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart#...
runtime/lib/bigint.dart:210: if (length > 0 && (digits == null || length >
digits.length)) {
You should remove the term digits == null.

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart#...
runtime/lib/bigint.dart:994: if ((_used > 2) || ((_used == 2) && (_digits[1] >
0x10000000))) {
My understanding is that we only use extra parenthesis in C++, not in Dart. I
may not be consistent everywhere.

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

https://codereview.chromium.org/580903004/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16346: ASSERT(TypedData::Handle(digits()).Length() == 0);
Is that good enough? We want the value to be canonical, so it should be
TypedData::EmptyUint32Array(isolate). If not, we should set it, but I suppose it
is.

[srdjan, 2014-09-18 20:59:19]

Thanks and PTAL

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart#...
runtime/lib/bigint.dart:77: }
On 2014/09/18 20:18:22, regis wrote:
> If I understand correctly, this indirection only verifies a correct
> implementation of _Bigint, but it is not free, unless the optimizer can
> eliminate it. I would remove it either now or when we declare the fields in
> Dart.

Inliner eliminates the indirection; I will add this to inlining white list as
well, making sure it is free. The setters/getters will be removed once we add
fields.

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart#...
runtime/lib/bigint.dart:210: if (length > 0 && (digits == null || length >
digits.length)) {
On 2014/09/18 20:18:22, regis wrote:
> You should remove the term digits == null.

Done.

https://codereview.chromium.org/580903004/diff/60001/runtime/lib/bigint.dart#...
runtime/lib/bigint.dart:994: if ((_used > 2) || ((_used == 2) && (_digits[1] >
0x10000000))) {
On 2014/09/18 20:18:22, regis wrote:
> My understanding is that we only use extra parenthesis in C++, not in Dart. I
> may not be consistent everywhere.

I think we should use it in all VM related code (i.e., where we have an
influence) since the problems are the same.

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

https://codereview.chromium.org/580903004/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16346: ASSERT(TypedData::Handle(digits()).Length() == 0);
On 2014/09/18 20:18:22, regis wrote:
> Is that good enough? We want the value to be canonical, so it should be
> TypedData::EmptyUint32Array(isolate). If not, we should set it, but I suppose
it
> is.

Done.

[regis, 2014-09-18 21:13:08]

LGTM

[srdjan, 2014-09-18 21:18:32]

Committed patchset #5 (id:80001) manually as r40463 (presubmit successful).