Fix Issue 1846: Improve error reporting in presence of imports. With this CL:

Fix Issue 1846: Improve error reporting in presence of imports. With this CL:

'/Users/srdjan/bdartall/dart/runtime/bug3.dart': Error: line 2 pos 1: unexpected token 'abstract'
abstract class A { }
^
'/Users/srdjan/bdartall/dart/runtime/bug2.dart': Error: line 2 pos 1: library handler failed
#import("bug3.dart");
^
'/Users/srdjan/bdartall/dart/runtime/bug1.dart': Error: line 2 pos 1: library handler failed
#import("bug2.dart");
^
Committed: https://code.google.com/p/dart/source/detail?r=4773

[hausner, 2012-02-29 22:56:02]

LokTM but it hurts a little. Feel free to ignore comment if you don't feel pain.

https://chromiumcodereview.appspot.com/9559001/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9559001/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3314: Dart_GetError(result), token_pos, "library handler
failed");
Dart_GetError() copies the error message out of an error object into a C string,
then you pass it to to a function that copies the C string into a Dart string
which is copied again when the old and new error message concatenated. 

This can probably be done with one less indirection. What if you pass the error
object into AppendErrorMsg()?

[srdjan, 2012-02-29 23:47:04]

https://chromiumcodereview.appspot.com/9559001/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9559001/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3314: Dart_GetError(result), token_pos, "library handler
failed");
On 2012/02/29 22:56:02, hausner wrote:
> Dart_GetError() copies the error message out of an error object into a C
string,
> then you pass it to to a function that copies the C string into a Dart string
> which is copied again when the old and new error message concatenated. 
> 
> This can probably be done with one less indirection. What if you pass the
error
> object into AppendErrorMsg()?

Done.

[hausner, 2012-03-01 00:10:16]

Post-mortem comment.

https://chromiumcodereview.appspot.com/9559001/diff/2002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9559001/diff/2002/runtime/vm/parser.cc...
runtime/vm/parser.cc:5270: const String& msg1 =
String::Handle(String::New(prev_error.ToErrorCString()));
Why not use prev_error.message()? There is still a detour through C land in your
version.

[srdjan, 2012-03-01 00:17:34]

https://chromiumcodereview.appspot.com/9559001/diff/2002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9559001/diff/2002/runtime/vm/parser.cc...
runtime/vm/parser.cc:5270: const String& msg1 =
String::Handle(String::New(prev_error.ToErrorCString()));
On 2012/03/01 00:10:16, hausner wrote:
> Why not use prev_error.message()? There is still a detour through C land in
your
> version.

There is no Error::message(). Some subclasses implement it, some don't. All
implement ToErrorCString, although some have UNREACHABLE in them.

[turnidge, 2012-03-07 00:41:20]

https://chromiumcodereview.appspot.com/9559001/diff/2002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9559001/diff/2002/runtime/vm/parser.cc...
runtime/vm/parser.cc:5270: const String& msg1 =
String::Handle(String::New(prev_error.ToErrorCString()));
On 2012/03/01 00:17:34, srdjan wrote:
> On 2012/03/01 00:10:16, hausner wrote:
> > Why not use prev_error.message()? There is still a detour through C land in
> your
> > version.
> 
> There is no Error::message(). Some subclasses implement it, some don't. All
> implement ToErrorCString, although some have UNREACHABLE in them.

You should be able to use ToErrorCString on any real Error object.  Nobody is
using UnwindErrors yet, so the UNIMPLEMENTED there doesn't matter.  All other
subclasses of Error have reasonable implementations.