Cleanup VM error handling.

runtime/lib/error.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "lib/error.h"
 
 #include "vm/bootstrap_natives.h"
 #include "vm/exceptions.h"
 #include "vm/object_store.h"
 #include "vm/runtime_entry.h"
 #include "vm/stack_frame.h"
 
 namespace dart {
 
 DEFINE_FLAG(bool, trace_type_checks, false, "Trace runtime type checks.");
 
 // Allocate and throw a new AssertionError.
 // Arg0: index of the first token of the failed assertion.
 // Arg1: index of the first token after the failed assertion.
 // Return value: none, throws an exception.
 DEFINE_NATIVE_ENTRY(AssertionError_throwNew, 2) {
   // No need to type check the arguments. This function can only be called
   // internally from the VM.
   intptr_t assertion_start =
       Smi::CheckedHandle(arguments->NativeArgAt(0)).Value();
   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();
 }
 
 
 // Allocate and throw a new TypeError.
 // Arg0: index of the token of the failed type check.
 // Arg1: src value.
 // Arg2: dst type name.
 // Arg3: dst name.
@@ skipping 18 matching lines @@
 }
 
 
 // Allocate and throw a new FallThroughError.
 // Arg0: index of the case clause token into which we fall through.
 // Return value: none, throws an exception.
 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();
 }
 
 
 // Allocate and throw a new AbstractClassInstantiationError.
 // Arg0: Token position of allocation statement.
 // Arg1: class name of the abstract class that cannot be instantiated.
 // Return value: none, throws an exception.
 DEFINE_NATIVE_ENTRY(AbstractClassInstantiationError_throwNew, 2) {
   GET_NON_NULL_NATIVE_ARGUMENT(Smi, smi_pos, arguments->NativeArgAt(0));
   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();
 }
 
 }  // namespace dart