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

vm/dart_api_impl.cc

Index: vm/dart_api_impl.cc
===================================================================
--- vm/dart_api_impl.cc	(revision 849)
+++ vm/dart_api_impl.cc	(working copy)
@@ -853,7 +853,18 @@
   Zone zone;  // Setup a VM zone as we are creating some handles.
   HandleScope scope;  // Setup a VM handle scope.
   const Object& obj = Object::Handle(Api::UnwrapHandle(object));
-  return obj.IsArray();
+  if (obj.IsArray()) {
+    return true;
+  }
+  // TODO(5526318): Make access to GrowableObjectArray more efficient.
+  if (obj.IsInstance()) {

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

Maybe extract this into an ImplementsListInterface helper?

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

Done.

+    Instance& instance = Instance::Handle();
+    instance ^= obj.raw();
+    Isolate* isolate = Isolate::Current();
+    const Type& type = Type::Handle(isolate->object_store()->list_interface());
+    return instance.Is(type);

[turnidge, 2011/10/27 21:29:46]

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?

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

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:

+  }
+  return false;
 }
 
 
@@ -875,10 +886,93 @@
     *len = array_obj.Length();
     return Api::Success();
   }
-  return Api::Error("Object is not an Array");
+  // TODO(5526318): Make access to GrowableObjectArray more efficient.
+  // Now check and handle a dart object that implements the List interface.
+  if (obj.IsInstance()) {
+    Instance& instance = Instance::Handle();
+    instance ^= obj.raw();
+    Isolate* isolate = Isolate::Current();
+    ASSERT(isolate != NULL);
+    const Type& type = Type::Handle(isolate->object_store()->list_interface());
+    if (instance.Is(type)) {
+      String& name = String::Handle(String::New("length"));
+      name = Field::GetterName(name);
+      const Function& function = Function::Handle(
+          Resolver::ResolveDynamic(instance, name, 1, 0));
+      if (!function.IsNull()) {
+        GrowableArray<const Object*> args(0);
+        LongJump* base = isolate->long_jump_base();
+        LongJump jump;
+        isolate->set_long_jump_base(&jump);
+        Dart_Handle result;
+        if (setjmp(*jump.Set()) == 0) {
+          const Array& kNoArgumentNames = Array::Handle();
+          const Instance& retval = Instance::Handle(
+              DartEntry::InvokeDynamic(instance,
+                                       function,
+                                       args,
+                                       kNoArgumentNames));
+          result = Api::Success();
+          if (retval.IsSmi() || retval.IsMint()) {
+            Integer& integer = Integer::Handle();
+            integer ^= retval.raw();
+            *len = integer.AsInt64Value();
+          } else if (retval.IsBigint()) {
+            Bigint& bigint = Bigint::Handle();
+            bigint ^= retval.raw();
+            if (BigintOperations::FitsIntoInt64(bigint)) {
+              *len = BigintOperations::ToInt64(bigint);
+            } else {
+              result = Api::Error("Object has an Invalid length");

[turnidge, 2011/10/27 21:29:46]

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."

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

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:

+            }
+          } else {
+            result = Api::Error("Object has an Invalid length");

[turnidge, 2011/10/27 21:29:46]

Invalid -> "non-integer"?

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

Done.

+          }
+        } else {
+          SetupErrorResult(&result);
+        }
+        isolate->set_long_jump_base(base);
+        return result;
+      }
+    }
+  }
+  return Api::Error("Object does not implement the list inteface");
 }
 
 
+static RawObject* GetArrayAt(const Instance& instance,
+                             const Integer& index,
+                             const Function& function,
+                             Dart_Handle* result) {
+  Isolate* isolate = Isolate::Current();
+  ASSERT(isolate != NULL);
+  ASSERT(result != NULL);
+  LongJump* base = isolate->long_jump_base();

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

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]

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:

+  LongJump jump;
+  isolate->set_long_jump_base(&jump);
+  if (setjmp(*jump.Set()) == 0) {
+    Instance& retval = Instance::Handle();
+    GrowableArray<const Object*> args(0);
+    args.Add(&index);
+    const Array& kNoArgumentNames = Array::Handle();
+    retval = DartEntry::InvokeDynamic(instance,
+                                      function,
+                                      args,
+                                      kNoArgumentNames);
+    if (retval.IsUnhandledException()) {
+      *result = Api::Error("Invalid implementation of '[]'");

[turnidge, 2011/10/27 21:29:46]

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.

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

Done.

+    } else {
+      *result = Api::Success();
+    }
+    isolate->set_long_jump_base(base);
+    return retval.raw();
+  }
+  SetupErrorResult(result);
+  isolate->set_long_jump_base(base);
+  return Object::null();
+}
+
+
 DART_EXPORT Dart_Handle Dart_ArrayGet(Dart_Handle array,
                                       intptr_t offset,
                                       uint8_t* native_array,
@@ -904,7 +998,39 @@
     }
     return Api::Error("Invalid length passed in to access array elements");
   }
-  return Api::Error("Object is not an Array");
+  // TODO(5526318): Make access to GrowableObjectArray more efficient.
+  // Now check and handle a dart object that implements the List interface.
+  if (obj.IsInstance()) {
+    Instance& instance = Instance::Handle();
+    instance ^= obj.raw();
+    Isolate* isolate = Isolate::Current();
+    ASSERT(isolate != NULL);
+    const Type& type = Type::Handle(isolate->object_store()->list_interface());
+    if (instance.Is(type)) {
+      String& name = String::Handle(String::New("[]"));

[turnidge, 2011/10/27 21:29:46]

An aside: it will be nice when we can start using named versions of these common
strings.

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

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:

+      const Function& function = Function::Handle(
+          Resolver::ResolveDynamic(instance, name, 2, 0));
+      if (!function.IsNull()) {
+        Object& element = Object::Handle();
+        Integer& intobj = Integer::Handle();
+        Dart_Handle result;
+        for (int i = 0; i < length; i++) {
+          intobj = Integer::New(offset + i);
+          element = GetArrayAt(instance, intobj, function, &result);
+          if (!Dart_IsValid(result)) {
+            return result;  // Error condition.
+          }
+          intobj ^= element.raw();
+          ASSERT(intobj.AsInt64Value() <= 0xff);
+          // TODO(hpayer): value should always be smaller then 0xff. Add error
+          // handling.
+          native_array[i] = static_cast<uint8_t>(intobj.AsInt64Value() & 0xff);
+        }
+        return Api::Success();
+      }
+    }
+  }
+  return Api::Error("Object does not implement the List interface");
 }
 
 
@@ -921,10 +1047,68 @@
     }
     return Api::Error("Invalid index passed in to access array element");
   }
+  // TODO(5526318): Make access to GrowableObjectArray more efficient.
+  // Now check and handle a dart object that implements the List interface.
+  if (obj.IsInstance()) {
+    Instance& instance = Instance::Handle();
+    instance ^= obj.raw();
+    Isolate* isolate = Isolate::Current();
+    ASSERT(isolate != NULL);
+    const Type& type = Type::Handle(isolate->object_store()->list_interface());
+    if (instance.Is(type)) {
+      String& name = String::Handle(String::New("[]"));
+      const Function& function = Function::Handle(
+          Resolver::ResolveDynamic(instance, name, 2, 0));
+      if (!function.IsNull()) {
+        Object& element = Object::Handle();
+        Integer& indexobj = Integer::Handle();
+        Dart_Handle result;
+        indexobj = Integer::New(index);
+        element = GetArrayAt(instance, indexobj, function, &result);
+        if (!Dart_IsValid(result)) {
+          return result;  // Error condition.
+        }
+        return Api::NewLocalHandle(element);
+      }
+    }
+  }
   return Api::Error("Object is not an Array");

[turnidge, 2011/10/27 21:29:46]

Inconsistent w/ error msg above -> "Object does not implement the List
interface".

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

Done.

 }
 
 
+static void SetArrayAt(const Instance& instance,
+                       const Integer& index,
+                       const Object& value,
+                       const Function& function,
+                       Dart_Handle* result) {
+  Isolate* isolate = Isolate::Current();
+  ASSERT(isolate != NULL);
+  ASSERT(result != NULL);
+  LongJump* base = isolate->long_jump_base();
+  LongJump jump;
+  isolate->set_long_jump_base(&jump);
+  if (setjmp(*jump.Set()) == 0) {
+    GrowableArray<const Object*> args(1);
+    args.Add(&index);
+    args.Add(&value);
+    Instance& retval = Instance::Handle();
+    const Array& kNoArgumentNames = Array::Handle();
+    retval = DartEntry::InvokeDynamic(instance,
+                                      function,
+                                      args,
+                                      kNoArgumentNames);
+    if (retval.IsUnhandledException()) {
+      *result = Api::Error("Invalid implementation of '[]='");

[turnidge, 2011/10/27 21:29:46]

Maybe different error msg here?

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

Done.

+    } else {
+      *result = Api::Success();
+    }
+  } else {
+    SetupErrorResult(result);
+  }
+  isolate->set_long_jump_base(base);
+}
+
+
 DART_EXPORT Dart_Handle Dart_ArraySet(Dart_Handle array,
                                       intptr_t offset,
                                       uint8_t* native_array,
@@ -945,9 +1129,38 @@
     }
     return Api::Error("Invalid length passed in to set array elements");
   }
-  return Api::Error("Object is not an Array");
+  // TODO(5526318): Make access to GrowableObjectArray more efficient.
+  // Now check and handle a dart object that implements the List interface.
+  if (obj.IsInstance()) {
+    Instance& instance = Instance::Handle();
+    instance ^= obj.raw();
+    Isolate* isolate = Isolate::Current();
+    ASSERT(isolate != NULL);
+    const Type& type = Type::Handle(isolate->object_store()->list_interface());

[turnidge, 2011/10/27 21:29:46]

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.

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

Done.

+    if (instance.Is(type)) {
+      String& name = String::Handle(String::New("[]="));
+      const Function& function = Function::Handle(
+          Resolver::ResolveDynamic(instance, name, 3, 0));
+      if (!function.IsNull()) {
+        Integer& indexobj = Integer::Handle();
+        Integer& valueobj = Integer::Handle();
+        Dart_Handle result;
+        for (int i = 0; i < length; i++) {
+          indexobj = Integer::New(offset + i);
+          valueobj ^= Integer::New(native_array[i]);
+          SetArrayAt(instance, indexobj, valueobj, function, &result);
+          if (!Dart_IsValid(result)) {
+            return result;  // Error condition.
+          }
+        }
+        return Api::Success();
+      }
+    }
+  }
+  return Api::Error("Object does not implement the list interface");
 }
 
+
 DART_EXPORT Dart_Handle Dart_ArraySetAt(Dart_Handle array,
                                         intptr_t index,
                                         Dart_Handle value) {
@@ -964,7 +1177,28 @@
     }
     return Api::Error("Invalid index passed in to set array element");
   }
-  return Api::Error("Object is not an Array");
+  // TODO(5526318): Make access to GrowableObjectArray more efficient.
+  // Now check and handle a dart object that implements the List interface.
+  if (obj.IsInstance()) {
+    Instance& instance = Instance::Handle();
+    instance ^= obj.raw();
+    Isolate* isolate = Isolate::Current();
+    ASSERT(isolate != NULL);
+    const Type& type = Type::Handle(isolate->object_store()->list_interface());
+    if (instance.Is(type)) {
+      String& name = String::Handle(String::New("[]="));
+      const Function& function = Function::Handle(
+          Resolver::ResolveDynamic(instance, name, 3, 0));
+      if (!function.IsNull()) {
+        Dart_Handle result;
+        const Integer& index_obj = Integer::Handle(Integer::New(index));
+        const Object& value_obj = Object::Handle(Api::UnwrapHandle(value));
+        SetArrayAt(instance, index_obj, value_obj, function, &result);
+        return result;
+      }
+    }
+  }
+  return Api::Error("Object does not implement the list interface");
 }