Better breakpoint management

Better breakpoint management

If a function is already compiled, make a source breakpoint at
the actual breakpoint line number, which may be different from the
requested line number.

Fixes an issue that was brought up in bug 3782.
Committed: https://code.google.com/p/dart/source/detail?r=10027

[siva, 2012-07-30 22:17:15]

lgtm

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

http://codereview.chromium.org/10839002/diff/1/runtime/vm/debugger.cc#newcode873
runtime/vm/debugger.cc:873: if (bpt != NULL) {
Why is it not necessary to ensure that the breakpoint is enabled in this case?

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

http://codereview.chromium.org/10839002/diff/1/runtime/vm/object.cc#newcode5103
runtime/vm/object.cc:5103: intptr_t* last_token_index) const {
Something to consider for later, not this CL:

we should agree on what token_index and token_pos mean and use these names
consistently everywhere.
I have always thought of token_index as something which represents an index into
the token stream object for the token and token_pos to mean the position of the
token in the source (we could also call it token_src_pos).
In this scheme the signature of scanner.TokenRangeAtLine would document the
paramters as first_token_pos and last_token_pos.

[hausner, 2012-07-30 22:36:57]

Thank you.

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

http://codereview.chromium.org/10839002/diff/1/runtime/vm/debugger.cc#newcode873
runtime/vm/debugger.cc:873: if (bpt != NULL) {
On 2012/07/30 22:17:15, asiva wrote:
> Why is it not necessary to ensure that the breakpoint is enabled in this case?
Because the caller will make sure it gets enabled if necessary.

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

http://codereview.chromium.org/10839002/diff/1/runtime/vm/object.cc#newcode5103
runtime/vm/object.cc:5103: intptr_t* last_token_index) const {
I agree. I was confused too and didn't realize that the token_index_ in
PCDescriptors is now actually an offset into the token "blob."

I would suggest to consistently use "index" for the token number in the source,
and pos for the offset into the token blob. The textual position (line, col, or
character index in the text) is also a pos, but token_pos is not the same as
text_pos. Am I making it confusing enough?

Definitely separate change.

 

On 2012/07/30 22:17:15, asiva wrote:
> Something to consider for later, not this CL:
> 
> we should agree on what token_index and token_pos mean and use these names
> consistently everywhere.
> I have always thought of token_index as something which represents an index
into
> the token stream object for the token and token_pos to mean the position of
the
> token in the source (we could also call it token_src_pos).
> In this scheme the signature of scanner.TokenRangeAtLine would document the
> paramters as first_token_pos and last_token_pos.
>