Cleanup VM error handling.

Cleanup VM error handling.

Unify the way the runtime throws errors. With this patch the VM goes through one location and always invokes the constructor of the errors.

It also makes it easier to rename fields in the error classes.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24995

Reverted: https://code.google.com/p/dart/source/detail?r=24997

Committed: https://code.google.com/p/dart/source/detail?r=25000

[siva, 2013-07-08 20:57:54]

Florian,

Could you explain how this cleanup makes things easier I am
probably missing the bigger picture here.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc
File runtime/lib/error.cc (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:29: const Array& args = Array::Handle(Array::New(4));
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 CL makes things easier, because:
- now every exception is created in the same place.
- the field names of the errors are independent of the CC code.
- the constructors of the errors are invoked.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc
File runtime/lib/error.cc (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:29: const Array& args = Array::Handle(Array::New(4));
On 2013/07/08 20:57:55, siva wrote:
> I find this hard coding of 4 based on the knowledge that
> Exceptions::SetLocationFields will return 3, as being a bit fragile.
This has nothing to do with SetLocationFields. It's the number of arguments the
constructor of the type-assertion takes.
> 
> 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.
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.

[floitsch, 2013-07-10 14:22:49]

ping.

[siva, 2013-07-12 16:49:35]

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc
File runtime/lib/error.cc (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:42: script.GetSnippet(from_line, from_column, to_line,
to_column)));
args.SetAt(0, ....);

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:47: ASSERT(args_index == args.Length());
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.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:99: ASSERT(args_index == args.Length());
See comment above regarding using 0 and 1 etc. instead of args_index

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:128: ASSERT(args_index == args.Length());
Ditto comment about args_index.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.dart
File runtime/lib/error.dart (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.dart#new...
runtime/lib/error.dart:118: final String _className;
The className field already exists in the super class why repeat it here?

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.dart#new...
runtime/lib/error.dart:120: final int _line;

If we are going with this scheme of using a Dart constructor then why not stream
line it some more by hoisting the url and line fields  up to "class Error" as
almost all error classes seem to declare these fields.

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.cc#n...
runtime/vm/exceptions.cc:465: ASSERT(args_index == args.Length());
ditto comment about using 0, 1, 2 etc. instead of args_index

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.h#ne...
runtime/vm/exceptions.h:43: intptr_t location);
This method not needed anymore....

[floitsch, 2013-07-12 17:12:35]

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc
File runtime/lib/error.cc (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:42: script.GetSnippet(from_line, from_column, to_line,
to_column)));
On 2013/07/12 16:49:36, siva wrote:
> args.SetAt(0, ....);

Done.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:47: ASSERT(args_index == args.Length());
On 2013/07/12 16:49:36, siva wrote:
> 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.

Done.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:99: ASSERT(args_index == args.Length());
On 2013/07/12 16:49:36, siva wrote:
> See comment above regarding using 0 and 1 etc. instead of args_index

Done.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.cc#newco...
runtime/lib/error.cc:128: ASSERT(args_index == args.Length());
On 2013/07/12 16:49:36, siva wrote:
> Ditto comment about args_index.

Done.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.dart
File runtime/lib/error.dart (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.dart#new...
runtime/lib/error.dart:118: final String _className;
On 2013/07/12 16:49:36, siva wrote:
> The className field already exists in the super class why repeat it here?

done.

https://codereview.chromium.org/18531003/diff/4001/runtime/lib/error.dart#new...
runtime/lib/error.dart:120: final int _line;
On 2013/07/12 16:49:36, siva wrote:
> 
> If we are going with this scheme of using a Dart constructor then why not
stream
> line it some more by hoisting the url and line fields  up to "class Error" as
> almost all error classes seem to declare these fields.

Sure. We can discuss this.
Not in this CL, though.

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.cc#n...
runtime/vm/exceptions.cc:465: ASSERT(args_index == args.Length());
On 2013/07/12 16:49:36, siva wrote:
> ditto comment about using 0, 1, 2 etc. instead of args_index

Done.

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (right):

https://codereview.chromium.org/18531003/diff/4001/runtime/vm/exceptions.h#ne...
runtime/vm/exceptions.h:43: intptr_t location);
On 2013/07/12 16:49:36, siva wrote:
> This method not needed anymore....

Done.

[siva, 2013-07-12 17:15:54]

lgtm

[floitsch, 2013-07-15 09:28:38]

Committed patchset #4 manually as r24995 (presubmit successful).

[floitsch, 2013-07-15 10:32:46]

Committed patchset #5 manually as r25000 (presubmit successful).