Enhance the array access API to deal with any objct that implements the list interface.

Enhance the array access API to deal with any objct that implements the list interface.

A future change will consider making the growable array a first class VM object so that access to growable array would be more efficient through the API.
Committed: https://code.google.com/p/dart/source/detail?r=972

[turnidge, 2011-10-27 21:29:45]

LGTM w/ comments.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode865
vm/dart_api_impl.cc:865: return instance.Is(type);
A thought.  Given that all of the other Is*() functions (like IsInstance, IsNum,
IsInteger, etc.) are at the Object level of the class hierarchy, we could
arguably move the Is(type) function up to that level as well (by default returns
false).   The advantage would be that you wouldn't have to do the IsInstance
check and the handle assignment here, making this kind of check more convenient
and (possibly) efficient.

What do you think?

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode926
vm/dart_api_impl.cc:926: result = Api::Error("Object has an Invalid length");
Is it possible to create a List at the Dart level that appears to be very large
(supports BigInt indices) but is actually compact (e.g. boolean array
IsPrimeUpToBazillion)?  List is just an interface and they could do whatever
they want.  That said, I don't think it is likely and we may want to make our
lives easier here.

About the error message, instead of "invalid", how about something like "List
length must be less that 2^64 or something like that."

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode929
vm/dart_api_impl.cc:929: result = Api::Error("Object has an Invalid length");
Invalid -> "non-integer"?

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode963
vm/dart_api_impl.cc:963: *result = Api::Error("Invalid implementation of '[]'");
What do you think of a more informative error message... something like
"Unexpected exception in []" or something.  It is also possible to pass along
the exception string in the error message if we want, but a bit harder.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1010
vm/dart_api_impl.cc:1010: String& name = String::Handle(String::New("[]"));
An aside: it will be nice when we can start using named versions of these common
strings.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1075
vm/dart_api_impl.cc:1075: return Api::Error("Object is not an Array");
Inconsistent w/ error msg above -> "Object does not implement the List
interface".

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1101
vm/dart_api_impl.cc:1101: *result = Api::Error("Invalid implementation of
'[]='");
Maybe different error msg here?

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1139
vm/dart_api_impl.cc:1139: const Type& type =
Type::Handle(isolate->object_store()->list_interface());
We are checking whether an object is a list many times in this file.  Maybe an
IsList helper function would tighten up the code a bit.

[Mads Ager (google), 2011-10-28 07:50:18]

LGTM

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode860
vm/dart_api_impl.cc:860: if (obj.IsInstance()) {
Maybe extract this into an ImplementsListInterface helper?

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode950
vm/dart_api_impl.cc:950: LongJump* base = isolate->long_jump_base();
Maybe a scoped object would be nice for this at some point?

SetLongJumpScope scope;
if (scope->setjmp() == 0) {
  ...
}

That would remove the need to remember to reset the base at all exits.

Not suggesting you do that for this change but I think it could make this type
of code smaller and easier to write.

[siva, 2011-10-31 20:30:49]

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode860
vm/dart_api_impl.cc:860: if (obj.IsInstance()) {
On 2011/10/28 07:50:18, Mads Ager wrote:
> Maybe extract this into an ImplementsListInterface helper?

Done.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode865
vm/dart_api_impl.cc:865: return instance.Is(type);
Regis and I discussed this, he felt that the only place where we would use
If(type) would be from the API functions
and he argued that since most of the objects handed out from
the Dart API should be instances it seems redundant to add
an Is(..) at the Object level.

Maybe we should have a UnWrapHandleAsInstance method which would return an error
if the handle does not contain an Instance and then just use
const Instance& instance = Instance::Handle(Api::UnwrapHandle(object));

Is(type) would just work and your concern about creating an additional handle
would also be addressed.

On 2011/10/27 21:29:46, turnidge wrote:
> A thought.  Given that all of the other Is*() functions (like IsInstance,
IsNum,
> IsInteger, etc.) are at the Object level of the class hierarchy, we could
> arguably move the Is(type) function up to that level as well (by default
returns
> false).   The advantage would be that you wouldn't have to do the IsInstance
> check and the handle assignment here, making this kind of check more
convenient
> and (possibly) efficient.
> 
> What do you think?

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode926
vm/dart_api_impl.cc:926: result = Api::Error("Object has an Invalid length");
Yes it is possible that an arbitrary List implementation could end up with a
true bigint as the index but I think in that case they could invoke the "length"
function and get that object back. In the context of this function we can assume
the index fits in intptr_t. So if does not fit it would be an error.

I have changed the error message per your suggestion.

On 2011/10/27 21:29:46, turnidge wrote:
> Is it possible to create a List at the Dart level that appears to be very
large
> (supports BigInt indices) but is actually compact (e.g. boolean array
> IsPrimeUpToBazillion)?  List is just an interface and they could do whatever
> they want.  That said, I don't think it is likely and we may want to make our
> lives easier here.
> 
> About the error message, instead of "invalid", how about something like "List
> length must be less that 2^64 or something like that."

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode929
vm/dart_api_impl.cc:929: result = Api::Error("Object has an Invalid length");
On 2011/10/27 21:29:46, turnidge wrote:
> Invalid -> "non-integer"?

Done.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode950
vm/dart_api_impl.cc:950: LongJump* base = isolate->long_jump_base();
Yes Todd had indicated the same thing, he is going to make a sweep over the
source base to see where setjmp is used and replace with SetLongJumpScope. This
will be done in another CL.

On 2011/10/28 07:50:18, Mads Ager wrote:
> Maybe a scoped object would be nice for this at some point?
> 
> SetLongJumpScope scope;
> if (scope->setjmp() == 0) {
>   ...
> }
> 
> That would remove the need to remember to reset the base at all exits.
> 
> Not suggesting you do that for this change but I think it could make this type
> of code smaller and easier to write.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode963
vm/dart_api_impl.cc:963: *result = Api::Error("Invalid implementation of '[]'");
On 2011/10/27 21:29:46, turnidge wrote:
> What do you think of a more informative error message... something like
> "Unexpected exception in []" or something.  It is also possible to pass along
> the exception string in the error message if we want, but a bit harder.

Done.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1010
vm/dart_api_impl.cc:1010: String& name = String::Handle(String::New("[]"));
Agree, there is a TODO somewhere in the VM code for this.
In fact we could also canonicalize these so that is it not necessary to do a
String::New(..);

On 2011/10/27 21:29:46, turnidge wrote:
> An aside: it will be nice when we can start using named versions of these
common
> strings.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1075
vm/dart_api_impl.cc:1075: return Api::Error("Object is not an Array");
On 2011/10/27 21:29:46, turnidge wrote:
> Inconsistent w/ error msg above -> "Object does not implement the List
> interface".

Done.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1101
vm/dart_api_impl.cc:1101: *result = Api::Error("Invalid implementation of
'[]='");
On 2011/10/27 21:29:46, turnidge wrote:
> Maybe different error msg here?

Done.

http://codereview.chromium.org/8417003/diff/7002/vm/dart_api_impl.cc#newcode1139
vm/dart_api_impl.cc:1139: const Type& type =
Type::Handle(isolate->object_store()->list_interface());
On 2011/10/27 21:29:46, turnidge wrote:
> We are checking whether an object is a list many times in this file.  Maybe an
> IsList helper function would tighten up the code a bit.

Done.