Add an interface for retrieving the value of unsigned 64-bit integers.

Add an interface for retrieving the value of unsigned 64-bit integers.

With this change, IntegerValue is renamed to IntegerToInt64 for symmetry
with the new IntegerToUint64 interface.  Also uses of UInt64 have been
replaced with Uint64 for naming consistency.

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

[turnidge, 2011-11-17 23:19:02]

Let's chat about naming a bit in person too.

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

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:742: uint64_t* value) {
Seeing "Uint64", I might actually prefer the form "UInt64", because in my mind I
think of it as Unsigned Int, instead of an opaque abbrevation.  It looks like we
were pretty consistent on UInt64 in the code before.  Thoughts?

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:762: return Api::Error("Integer too big to fit in
uint64_t");
Regarding the range checks here and above.

Not sure I personally think "small" when I see a large negative integer.  Also,
negative numbers will come out as "too big" here in the second test.  One could
hoist the negative test above out of the smi/mint case and say something about
"must be non-negative".

Whatever we do, we should consider making the IntegerToInt64 above have
consistent error messages.

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:770: const char** value) {
I think we should make this name consistent with the name above, possibly
changing to "IntegerToHexCString".

These become inconsistent with most of the value-accessors in this file,
unfortunately, so I could see these all going through another round of changes
down the line.

[cshapiro, 2011-11-18 10:06:54]

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

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:742: uint64_t* value) {
I agree, but I was the one who put the UInt into the code base and, in
retrospect, I was not being consistent with the rest of the system.  Plus, the
JavaScript API for Typed Arrays uses the Uint convention and so its in our Dart
code.  Better one convention than two.  But, there is no reason we cannot wholly
go either way on this.  I have no preference aside from consistency so I am
happy to flip everything in the UInt direction instead.

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:762: return Api::Error("Integer too big to fit in
uint64_t");
The missing negative test is an outright bug.  (That is what I get for not
writing a unit test for this API call.   I will fix that in the re-upload.) 
This is meant to be an access and not a conversion so it should never return a
value from thin air.

At any rate, I will lose the "too small" phrasing and put something (hopefully)
more accessible in its place.

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:770: const char** value) {
We should certainly try and reduce the amount of naming entropy.  I will go
ahead and make the change here as well.

[turnidge, 2011-11-22 21:06:14]

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

http://codereview.chromium.org/8585049/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:742: uint64_t* value) {
Okay, go ahead with Uint then.  That seems okay.

On 2011/11/18 10:06:54, cshapiro wrote:
> I agree, but I was the one who put the UInt into the code base and, in
> retrospect, I was not being consistent with the rest of the system.  Plus, the
> JavaScript API for Typed Arrays uses the Uint convention and so its in our
Dart
> code.  Better one convention than two.  But, there is no reason we cannot
wholly
> go either way on this.  I have no preference aside from consistency so I am
> happy to flip everything in the UInt direction instead.

[turnidge, 2011-12-08 18:26:25]

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:977: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
I've been doing things a new way.  Sorry I didn't notice this in earlier
reviews.  There is a new UnwrapBlahHandle that is helpful:

const Integer& obj = Integer::Handle(Api::UnwrapIntegerHandle(integer);
if (obj.isNull()) {
  RETURN_TYPE_ERROR(integer, Integer);
}

This will check for null and for non-integers and give a standard error message.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1015: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
Do you mind changing this to use UnwrapIntegerHandle?

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1029: return Api::Error("Integer too big to fit in
int64_t");
Now that I have printf args to Api::Error, I am trying to put the function name
first.  Also, is "too big" the right message?  Maybe even something like this:

return Api::Error("%s: parameter 'integer' has value '%s' which does not fit
into the range of an int64_t.", CURRENT_FUNC, obj.ToCString());

I realize this is an existing problem...

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1039: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
UnwrapIntegerHandle here too?

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1044: return Api::Error("Integer too small to fit in
uint64_t");
Fixup error message as suggested for IntegerToInt64 above.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1057: return Api::Error("Integer too big to fit in
uint64_t");
Fixup error message as above.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1067: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
UnwrapIntegerHandle here too?

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:319: UNIT_TEST_CASE(IntegerFitsInto) {
Could you make this test check 4 values for each type, one value on either side
of the border?

e.g. for signed: -1, 0, 0xFFFFFFFFFFFFFFFF, 0x10000000000000000

It might be easier for me to read if the int64 and uint64 tests were not
interleaved.

[cshapiro, 2011-12-08 22:50:50]

TAL

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:957: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
Done here as well.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:977: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
Done.

I will put together a separate change for Dart_BooleanValue and
Dart_DoubleValue.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1015: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
On 2011/12/08 18:26:25, turnidge wrote:
> Do you mind changing this to use UnwrapIntegerHandle?

Done.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1029: return Api::Error("Integer too big to fit in
int64_t");
Done.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1039: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
On 2011/12/08 18:26:25, turnidge wrote:
> UnwrapIntegerHandle here too?

Done.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1044: return Api::Error("Integer too small to fit in
uint64_t");
Done.  I have created a single call to error with a descriptive message.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1057: return Api::Error("Integer too big to fit in
uint64_t");
On 2011/12/08 18:26:25, turnidge wrote:
> Fixup error message as above.

Done.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1067: const Object& obj =
Object::Handle(Api::UnwrapHandle(integer));
On 2011/12/08 18:26:25, turnidge wrote:
> UnwrapIntegerHandle here too?

Done.

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8585049/diff/6001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:319: UNIT_TEST_CASE(IntegerFitsInto) {
Done.

[turnidge, 2011-12-08 22:58:21]

lgtm, minor comments...

http://codereview.chromium.org/8585049/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8585049/diff/11001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:959: return Api::Success();
Remove this?

http://codereview.chromium.org/8585049/diff/11001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1053: CURRENT_FUNC, int_obj.ToCString());
Make consistent with IntegerToInt64?  (That is, fall out to Api::Success
instead?)