Add token index position to classes and types for more accurate error reporting.

Add token index position to classes and types for more accurate error reporting.
Committed: https://code.google.com/p/dart/source/detail?r=3965

[siva, 2012-02-06 18:13:08]

LGTM once comment regarding default dummy token position value is made
consistent.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

http://codereview.chromium.org/9325047/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:46: const AbstractType& type);
Why not call it FinalizeAndCanonicalizeType ?

http://codereview.chromium.org/9325047/diff/1/runtime/vm/dart_api_impl_test.cc
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/9325047/diff/1/runtime/vm/dart_api_impl_test.c...
runtime/vm/dart_api_impl_test.cc:1498: "'dart:test-lib': Error: line 1 pos 38:
class 'NativeFields' is trying");
Why did you chop of the rest of the error regarding library not having native
resolvers. I think that is a useful test which validates we are not finding
native resolvers.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc#newcode832
runtime/vm/object.cc:832: // TODO(regis): Use dummy token index 1 for now; see
TODO above.
The comment says use dummy token index 1 but in the function
CreateAndRegisterInterface there is a comment stating dummy token index as 0.
Maybe you should have a const kDummyTokenIndex or something like that and use it
consistentnly here and in the other test case file too.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc#newcode874
runtime/vm/object.cc:874: const intptr_t kPos = 0;  // Dummy token index for
unspecified position.
Ditto comment regarding dummy token index.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc#newcode1295
runtime/vm/object.cc:1295: const intptr_t kPos = 0;  // Dummy token index for
unspecified position.
Ditto comment (0 or 1)?

[regis, 2012-02-06 19:43:16]

Thanks!

http://codereview.chromium.org/9325047/diff/1/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

http://codereview.chromium.org/9325047/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:46: const AbstractType& type);
On 2012/02/06 18:13:08, asiva wrote:
> Why not call it FinalizeAndCanonicalizeType ?

Not all types are canonicalized (e.g. ParameterType is not). The
"AndCanonicalize" suffix was used to differentiate the public version from the
private version of FinalizeType. We need only one version now and I picked the
more readable name. I have modified the comment about canonicalization.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/dart_api_impl_test.cc
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/9325047/diff/1/runtime/vm/dart_api_impl_test.c...
runtime/vm/dart_api_impl_test.cc:1498: "'dart:test-lib': Error: line 1 pos 38:
class 'NativeFields' is trying");
On 2012/02/06 18:13:08, asiva wrote:
> Why did you chop of the rest of the error regarding library not having native
> resolvers. I think that is a useful test which validates we are not finding
> native resolvers.

The emitted error message is now much longer, since it contains the exact
position as well as a snippet of the faulty code. I think that verifying only a
prefix of the error message is sufficient. Otherwise, I would have to copy
multiple lines of output in this test. I can make the prefix longer if you think
it is not testing enough in its current form.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc#newcode832
runtime/vm/object.cc:832: // TODO(regis): Use dummy token index 1 for now; see
TODO above.
On 2012/02/06 18:13:08, asiva wrote:
> The comment says use dummy token index 1 but in the function
> CreateAndRegisterInterface there is a comment stating dummy token index as 0.
> Maybe you should have a const kDummyTokenIndex or something like that and use
it
> consistentnly here and in the other test case file too.

I tried to use 0 for a non-existing source and 1 for an unknown source position.
But this is unnecessary and confusing. I now always use 0 as defined by
Scanner::kDummyTokenIndex.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc#newcode874
runtime/vm/object.cc:874: const intptr_t kPos = 0;  // Dummy token index for
unspecified position.
On 2012/02/06 18:13:08, asiva wrote:
> Ditto comment regarding dummy token index.

Done.

http://codereview.chromium.org/9325047/diff/1/runtime/vm/object.cc#newcode1295
runtime/vm/object.cc:1295: const intptr_t kPos = 0;  // Dummy token index for
unspecified position.
On 2012/02/06 18:13:08, asiva wrote:
> Ditto comment (0 or 1)?

Done.

[regis, 2012-02-06 20:07:48]

https://chromiumcodereview.appspot.com/9325047/diff/1/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9325047/diff/1/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:1498: "'dart:test-lib': Error: line 1 pos 38:
class 'NativeFields' is trying");
On 2012/02/06 19:43:16, regis wrote:
> On 2012/02/06 18:13:08, asiva wrote:
> > Why did you chop of the rest of the error regarding library not having
native
> > resolvers. I think that is a useful test which validates we are not finding
> > native resolvers.
> 
> The emitted error message is now much longer, since it contains the exact
> position as well as a snippet of the faulty code. I think that verifying only
a
> prefix of the error message is sufficient. Otherwise, I would have to copy
> multiple lines of output in this test. I can make the prefix longer if you
think
> it is not testing enough in its current form.

As discussed, I will not chop off the error message so short  as to test the
library url. I'll send you a cl.