Make Script::GetTokenLocation() and Script::TokenRangeAtLine() use token streams where possible.

Make Script::GetTokenLocation() and Script::TokenRangeAtLine() use token streams
where possible.

BUG=
R=hausner@google.com, iposva@google.com

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

[Michael Lippautz (Google), 2013-09-23 18:26:34]

Scanner::TokenRangeAtLine() becomes dead code, so we could remove it.

[hausner, 2013-09-23 18:54:49]

LGTM with comments.

Food for thought: would it make sense to add line numbers to the token iterator?
That would make it easier to introduce more sophisticated newline tokens at some
point, e.g. one that represents two or more newlines.

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6371: while (tkit.CurrentPosition() < token_pos) {
Does this terminate properly if I give a token position that is past the end of
the token stream?

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6417: *last_token_index = end_pos;
The old code only assigned to the out parameters if they are non-null.

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:2077: script.GetTokenLocation(0, &fast_line, NULL);
Can you add a comment here why you call GetTokenLocation in two different ways?
One day this might be obsolete and nobody remembers what the reason was... :-)

[Michael Lippautz (Google), 2013-09-23 21:15:03]

PTAL

Had to add a bit of code to get TokenRangeAtLine() compatible with the expected
behavior from the debugger.

Also, YES, adding a linecount to the iterator would make things easier here.
(Investigating in separate CL.)

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6371: while (tkit.CurrentPosition() < token_pos) {
On 2013/09/23 18:54:49, hausner wrote:
> Does this terminate properly if I give a token position that is past the end
of
> the token stream?

Fixed.

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6417: *last_token_index = end_pos;
On 2013/09/23 18:54:49, hausner wrote:
> The old code only assigned to the out parameters if they are non-null.

The out parameters are required to be non-NULL. Moved the assert (which was
hidden in scanner.TokenRangeAtLine) into this function.

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/24359005/diff/3001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:2077: script.GetTokenLocation(0, &fast_line, NULL);
On 2013/09/23 18:54:49, hausner wrote:
> Can you add a comment here why you call GetTokenLocation in two different
ways?
> One day this might be obsolete and nobody remembers what the reason was... :-)

Done.

[hausner, 2013-09-23 22:19:36]

LGTM with small nit.

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6415: // End of token stream before reaching required line.
This comment belongs inside the if block.

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6419: // No tokens on the current line. If there is a valid
token afterwards, put it
ditto

[Ivan Posva, 2013-09-23 22:53:34]

LGTM after addressing the line down counter.

-Ivan

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6371: intptr_t relative_line = 1;
Why don't you start with:
intptr_t cur_line = line_offset() + 1:

[Michael Lippautz (Google), 2013-09-23 23:24:00]

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/24359005/diff/10001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6371: intptr_t relative_line = 1;
On 2013/09/23 22:53:34, Ivan Posva wrote:
> Why don't you start with:
> intptr_t cur_line = line_offset() + 1:

Done.

[Michael Lippautz (Google), 2013-09-23 23:25:06]

Committed patchset #3 manually as r27786 (presubmit successful).