Cleanup api for isolate state checking.

runtime/vm/dart_api_impl.cc

Index: runtime/vm/dart_api_impl.cc
===================================================================
--- runtime/vm/dart_api_impl.cc	(revision 18006)
+++ runtime/vm/dart_api_impl.cc	(working copy)
@@ -79,29 +79,6 @@
   } while (0)
 
 
-// Return error if isolate is in an inconsistent state.
-// Return NULL when no error condition exists.
-//
-// TODO(turnidge): Make this function return an error handle directly
-// rather than returning an error string.  The current behavior can
-// cause compilation errors to appear to be api errors.
-const char* CheckIsolateState(Isolate* isolate) {
-  if (ClassFinalizer::FinalizePendingClasses() &&
-      isolate->object_store()->PreallocateObjects()) {
-    // Success.
-    return NULL;
-  }
-  // Make a copy of the error message as the original message string
-  // may get deallocated when we return back from the Dart API call.
-  const Error& err = Error::Handle(isolate->object_store()->sticky_error());
-  const char* errmsg = err.ToErrorCString();
-  intptr_t errlen = strlen(errmsg) + 1;
-  char* msg = Api::TopScope(isolate)->zone()->Alloc<char>(errlen);
-  OS::SNPrint(msg, errlen, "%s", errmsg);
-  return msg;
-}
-
-
 void SetupErrorResult(Isolate* isolate, Dart_Handle* handle) {
   *handle = Api::NewHandle(
       isolate, Isolate::Current()->object_store()->sticky_error());
@@ -176,6 +153,20 @@
 }
 
 
+Dart_Handle Api::CheckIsolateState(Isolate* isolate) {
+  if (ClassFinalizer::FinalizePendingClasses() &&
+      isolate->object_store()->PreallocateObjects()) {
+    return Api::Success(isolate);
+  }
+  const Object& obj = Object::Handle(isolate->object_store()->sticky_error());
+  if (obj.IsError()) {

[siva, 2013/02/01 22:56:26]

Shouldn't this always be an Error, in what cases can it not be an error.

Maybe we just assert it is an Error here.

[regis, 2013/02/01 23:43:16]

Done.

+    return Api::NewHandle(isolate, obj.raw());
+  }
+  ASSERT(obj.IsNull());
+  return Api::Success(isolate);
+}
+
+
 Dart_Isolate Api::CastIsolate(Isolate* isolate) {
   return reinterpret_cast<Dart_Isolate>(isolate);
 }
@@ -905,9 +896,9 @@
   if (size == NULL) {
     RETURN_NULL_ERROR(size);
   }
-  const char* msg = CheckIsolateState(isolate);
-  if (msg != NULL) {
-    return Api::NewError("%s", msg);
+  Dart_Handle state = Api::CheckIsolateState(isolate);
+  if (Dart_IsError(state)) {
+    return state;
   }
   // Since this is only a snapshot the root library should not be set.
   isolate->object_store()->set_root_library(Library::Handle(isolate));
@@ -929,9 +920,9 @@
   if (size == NULL) {
     RETURN_NULL_ERROR(size);
   }
-  const char* msg = CheckIsolateState(isolate);
-  if (msg != NULL) {
-    return Api::NewError("%s", msg);
+  Dart_Handle state = Api::CheckIsolateState(isolate);
+  if (Dart_IsError(state)) {
+    return state;
   }
   Library& library =
       Library::Handle(isolate, isolate->object_store()->root_library());
@@ -1255,9 +1246,9 @@
         CURRENT_FUNC);
   }
   // Finalize all classes.
-  const char* msg = CheckIsolateState(isolate);
-  if (msg != NULL) {
-    return Api::NewError("%s", msg);
+  Dart_Handle state = Api::CheckIsolateState(isolate);
+  if (Dart_IsError(state)) {
+    return state;
   }
   if (obj.IsInstance()) {
     CHECK_CALLBACK_STATE(isolate);
@@ -2530,9 +2521,9 @@
   }
 
   // Finalize all classes.
-  const char* msg = CheckIsolateState(isolate);
-  if (msg != NULL) {
-    return Api::NewError("%s", msg);
+  Dart_Handle state = Api::CheckIsolateState(isolate);
+  if (Dart_IsError(state)) {
+    return state;
   }
 
   const Array& interface_types = Array::Handle(isolate, cls.interfaces());
@@ -3296,9 +3287,9 @@
   } else {
     RETURN_TYPE_ERROR(isolate, constructor_name, String);
   }
-  const char* msg = CheckIsolateState(isolate);
-  if (msg != NULL) {
-    return Api::NewError("%s", msg);
+  Dart_Handle state = Api::CheckIsolateState(isolate);
+  if (Dart_IsError(state)) {
+    return state;
   }
 
   // Resolve the constructor.
@@ -3426,9 +3417,9 @@
 
   } else if (obj.IsClass()) {
     // Finalize all classes.
-    const char* msg = CheckIsolateState(isolate);
-    if (msg != NULL) {
-      return Api::NewError("%s", msg);
+    Dart_Handle state = Api::CheckIsolateState(isolate);
+    if (Dart_IsError(state)) {
+      return state;
     }
 
     const Class& cls = Class::Cast(obj);
@@ -3463,9 +3454,9 @@
 
     // Finalize all classes if needed.
     if (finalize_classes) {
-      const char* msg = CheckIsolateState(isolate);
-      if (msg != NULL) {
-        return Api::NewError("%s", msg);
+      Dart_Handle state = Api::CheckIsolateState(isolate);
+      if (Dart_IsError(state)) {
+        return state;
       }
     }
 
@@ -3555,9 +3546,9 @@
 
   } else if (obj.IsClass()) {
     // Finalize all classes.
-    const char* msg = CheckIsolateState(isolate);
-    if (msg != NULL) {
-      return Api::NewError("%s", msg);
+    Dart_Handle state = Api::CheckIsolateState(isolate);
+    if (Dart_IsError(state)) {
+      return state;
     }
     // To access a static field we may need to use the Field or the
     // getter Function.
@@ -4140,10 +4131,9 @@
 DART_EXPORT Dart_Handle Dart_CompileAll() {
   Isolate* isolate = Isolate::Current();
   DARTSCOPE(isolate);
-  Dart_Handle result;
-  const char* msg = CheckIsolateState(isolate);
-  if (msg != NULL) {
-    return Api::NewError("%s", msg);
+  Dart_Handle result = Api::CheckIsolateState(isolate);
+  if (Dart_IsError(result)) {
+    return result;
   }
   CHECK_CALLBACK_STATE(isolate);
   CompileAll(isolate, &result);
@@ -4293,9 +4283,9 @@
   // If this is the dart:builtin library, register it with the VM.
   if (url_str.Equals("dart:builtin")) {
     isolate->object_store()->set_builtin_library(library);
-    const char* msg = CheckIsolateState(isolate);
-    if (msg != NULL) {
-      return Api::NewError("%s", msg);
+    Dart_Handle state = Api::CheckIsolateState(isolate);
+    if (Dart_IsError(state)) {
+      return state;
     }
   }
   return result;