Cleanup VM error handling.

runtime/lib/error.cc

Index: runtime/lib/error.cc
diff --git a/runtime/lib/error.cc b/runtime/lib/error.cc
index f9794a0daf202c7645ee73837821d8db9d48163f..f25e03deeac38e3262182e8be15c053a32a0450d 100644
--- a/runtime/lib/error.cc
+++ b/runtime/lib/error.cc
@@ -26,27 +26,27 @@ DEFINE_NATIVE_ENTRY(AssertionError_throwNew, 2) {
   intptr_t assertion_end =
       Smi::CheckedHandle(arguments->NativeArgAt(1)).Value();
 
-  // Allocate a new instance of type AssertionError.
-  const Instance& assertion_error = Instance::Handle(
-      Exceptions::NewInstance("AssertionErrorImplementation"));
+  const Array& args = Array::Handle(Array::New(4));

[siva, 2013/07/08 20:57:55]

I find this hard coding of 4 based on the knowledge that
Exceptions::SetLocationFields will return 3, as being a bit fragile.

Somebody may change Exceptions::SetLocationFields to return 4 because they have
added yet another field without realizing that this value of '4' here needs to
be changed as well.

[floitsch, 2013/07/09 10:56:29]

This has nothing to do with SetLocationFields. It's the number of arguments the
constructor of the type-assertion takes.
Then the ASSERT in line 47 will trigger (unless some other check, which is not
unlikely, triggers before).

This is not worse than the old code, where changing the Dart files required to
change the cc-files. You would get a crash if you didn't.

+  intptr_t args_index = 0;
 
-  // Initialize 'url', 'line', and 'column' fields.
   DartFrameIterator iterator;
   iterator.NextFrame();  // Skip native call.
   const Script& script = Script::Handle(Exceptions::GetCallerScript(&iterator));
-  const Class& cls = Class::Handle(assertion_error.clazz());
-  Exceptions::SetLocationFields(assertion_error, cls, script, assertion_start);
 
-  // Initialize field 'failed_assertion' with source snippet.
+  // Initialize argument 'failed_assertion' with source snippet.
   intptr_t from_line, from_column;
   script.GetTokenLocation(assertion_start, &from_line, &from_column);
   intptr_t to_line, to_column;
   script.GetTokenLocation(assertion_end, &to_line, &to_column);
-  Exceptions::SetField(assertion_error, cls, "failedAssertion", String::Handle(
+  args.SetAt(args_index++, String::Handle(
       script.GetSnippet(from_line, from_column, to_line, to_column)));

[siva, 2013/07/12 16:49:36]

args.SetAt(0, ....);

[floitsch, 2013/07/12 17:12:36]

Done.

 
-  // Throw AssertionError instance.
-  Exceptions::Throw(assertion_error);
+  // Initialize location arguments starting at position 1.
+  args_index +=
+      Exceptions::SetLocationFields(args, args_index, script, assertion_start);
+  ASSERT(args_index == args.Length());

[siva, 2013/07/12 16:49:36]

I think this code is unreadable and hard to understand, I would prefer
that we got rid of Exceptions::SetLocationFields and instead used:

  args.SetAt(1, String::Handle(script.url()));
  intptr_t line, column;
  script.GetTokenLocation(location, &line, &column);
  args.SetAt(2, Smi::Handle(Smi::New(line)));
  args.SetAt(3, Smi::Handle(Smi::New(column)));

It would be more clear and the 4 above would be obvious.

[floitsch, 2013/07/12 17:12:36]

Done.

+
+  Exceptions::ThrowByType(Exceptions::kAssertion, args);
   UNREACHABLE();
   return Object::null();
 }
@@ -85,25 +85,20 @@ DEFINE_NATIVE_ENTRY(FallThroughError_throwNew, 1) {
   GET_NON_NULL_NATIVE_ARGUMENT(Smi, smi_pos, arguments->NativeArgAt(0));
   intptr_t fallthrough_pos = smi_pos.Value();
 
-  // Allocate a new instance of type FallThroughError.
-  const Instance& fallthrough_error = Instance::Handle(Exceptions::NewInstance(
-      "FallThroughErrorImplementation"));
-  ASSERT(!fallthrough_error.IsNull());
+  const Array& args = Array::Handle(Array::New(2));
+  intptr_t args_index = 0;
 
-  // Initialize 'url' and 'line' fields.
+  // Initialize 'url' and 'line' arguments.
   DartFrameIterator iterator;
   iterator.NextFrame();  // Skip native call.
   const Script& script = Script::Handle(Exceptions::GetCallerScript(&iterator));
-  const Class& cls = Class::Handle(fallthrough_error.clazz());
-  Exceptions::SetField(fallthrough_error, cls, "url",
-                       String::Handle(script.url()));
+  args.SetAt(args_index++, String::Handle(script.url()));
   intptr_t line, column;
   script.GetTokenLocation(fallthrough_pos, &line, &column);
-  Exceptions::SetField(fallthrough_error, cls, "line",
-                       Smi::Handle(Smi::New(line)));
+  args.SetAt(args_index++, Smi::Handle(Smi::New(line)));
+  ASSERT(args_index == args.Length());

[siva, 2013/07/12 16:49:36]

See comment above regarding using 0 and 1 etc. instead of args_index

[floitsch, 2013/07/12 17:12:36]

Done.

 
-  // Throw FallThroughError instance.
-  Exceptions::Throw(fallthrough_error);
+  Exceptions::ThrowByType(Exceptions::kFallThrough, args);
   UNREACHABLE();
   return Object::null();
 }
@@ -118,24 +113,21 @@ DEFINE_NATIVE_ENTRY(AbstractClassInstantiationError_throwNew, 2) {
   GET_NON_NULL_NATIVE_ARGUMENT(String, class_name, arguments->NativeArgAt(1));
   intptr_t error_pos = smi_pos.Value();
 
-  // Allocate a new instance of type AbstractClassInstantiationError.
-  const Instance& error = Instance::Handle(Exceptions::NewInstance(
-      "AbstractClassInstantiationErrorImplementation"));
-  ASSERT(!error.IsNull());
+  const Array& args = Array::Handle(Array::New(3));
+  intptr_t args_index = 0;
 
-  // Initialize 'url', 'line' and 'className' fields.
+  // Initialize 'className', 'url' and 'line' arguments.
   DartFrameIterator iterator;
   iterator.NextFrame();  // Skip native call.
   const Script& script = Script::Handle(Exceptions::GetCallerScript(&iterator));
-  const Class& cls = Class::Handle(error.clazz());
-  Exceptions::SetField(error, cls, "url", String::Handle(script.url()));
+  args.SetAt(args_index++, class_name);
+  args.SetAt(args_index++, String::Handle(script.url()));
   intptr_t line, column;
   script.GetTokenLocation(error_pos, &line, &column);
-  Exceptions::SetField(error, cls, "line", Smi::Handle(Smi::New(line)));
-  Exceptions::SetField(error, cls, "className", class_name);
+  args.SetAt(args_index++, Smi::Handle(Smi::New(line)));
+  ASSERT(args_index == args.Length());

[siva, 2013/07/12 16:49:36]

Ditto comment about args_index.

[floitsch, 2013/07/12 17:12:36]

Done.

 
-  // Throw AbstractClassInstantiationError instance.
-  Exceptions::Throw(error);
+  Exceptions::ThrowByType(Exceptions::kAbstractClassInstantiation, args);
   UNREACHABLE();
   return Object::null();
 }