Change locationFromPosition() and locationFromLine() to use a binary search t...

Change locationFromPosition() and locationFromLine() to use a binary search to locate line numbers from position numbers.  Modify test debug-sourceinfo.js to include more tests, including error conditions.

Ran the test successfully with the old and new version of messages.js.  This implies things work the same, including "error" conditions of supplying bad position numbers (that is, querying "<0" returns 0 while querying ">max" returns null).

[Søren Thygesen Gjesse, 2009-06-08 08:35:34]

LGTM

http://codereview.chromium.org/118371/diff/1/3
File src/messages.js (right):

http://codereview.chromium.org/118371/diff/1/3#newcode242
Line 242: // We'll never find invalid positions so bail right away
Please end the comment with a . (for all comments in this file).

http://codereview.chromium.org/118371/diff/1/3#newcode331
Line 331: if (offset_line == -1 || offset_line + line >= this.lineCount())
return null;
Line longer than 80 chars. Please use {}'s for all if statements.

http://codereview.chromium.org/118371/diff/1/2
File test/mjsunit/debug-sourceinfo.js (right):

http://codereview.chromium.org/118371/diff/1/2#newcode65
Line 65: // This is the last position in the entire file <debug-sourceinfo.js>
Please end the comment with a . (for all comments in this file).

http://codereview.chromium.org/118371/diff/1/2#newcode67
Line 67: var last_line = 338;
Please add a comment stating that the first line is line 0 (as the file actually
has 339 lines).

http://codereview.chromium.org/118371/diff/1/2#newcode130
Line 130: assertEquals(start_line_d + line, location.line);
Maybe add an assertion for location.column and location.position as well. I
think all the information for calculation their expected values should be easily
available.

[Matt Hanselman, 2009-06-09 03:29:54]

I believe I addressed all review comments.  I reworded a few of the source
comments since I was in there again, most notably in the function header for
messages.js.

I noticed many other lines in messages.js that are longer than 80 chars.  I
certainly don't mind cleaning that up if you think it's worthwhile; just let me
know to go another round.


On 2009/06/08 08:35:34, Søren Gjesse wrote:
> LGTM
> 
> http://codereview.chromium.org/118371/diff/1/3
> File src/messages.js (right):
> 
> http://codereview.chromium.org/118371/diff/1/3#newcode242
> Line 242: // We'll never find invalid positions so bail right away
> Please end the comment with a . (for all comments in this file).
> 
> http://codereview.chromium.org/118371/diff/1/3#newcode331
> Line 331: if (offset_line == -1 || offset_line + line >= this.lineCount())
> return null;
> Line longer than 80 chars. Please use {}'s for all if statements.
> 
> http://codereview.chromium.org/118371/diff/1/2
> File test/mjsunit/debug-sourceinfo.js (right):
> 
> http://codereview.chromium.org/118371/diff/1/2#newcode65
> Line 65: // This is the last position in the entire file <debug-sourceinfo.js>
> Please end the comment with a . (for all comments in this file).
> 
> http://codereview.chromium.org/118371/diff/1/2#newcode67
> Line 67: var last_line = 338;
> Please add a comment stating that the first line is line 0 (as the file
actually
> has 339 lines).
> 
> http://codereview.chromium.org/118371/diff/1/2#newcode130
> Line 130: assertEquals(start_line_d + line, location.line);
> Maybe add an assertion for location.column and location.position as well. I
> think all the information for calculation their expected values should be
easily
> available.