Transmit breakpoint id on paused event

Transmit breakpoint id on paused event

Breakpoint callback get an additional parameter to receive the id of the breakpoint that was hit, or 0 if the debugger pauses due to stepping.

R=turnidge@google.com

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

[turnidge, 2013-12-10 17:17:18]

lgtm with comments.

Thanks for making this change.

https://codereview.chromium.org/110913002/diff/20001/runtime/bin/dbg_message.cc
File runtime/bin/dbg_message.cc (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/bin/dbg_message....
runtime/bin/dbg_message.cc:1077: }
We could possibly use a named constant here instead of zero.  kNoBreakpoint or
kBreakStepMode or something.

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1884: SourceBreakpoint* breakpoint) {
Name is not consistent with 'bpt' used elsewhere in file.  Could consider making
them the same.

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.h#ne...
runtime/vm/debugger.h:272: Dart_Port isolate_id;
At some point this may grow out of being a struct and may be better as a tree of
classes.  Not now, but something to consider.

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger_api_...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger_api_...
runtime/vm/debugger_api_impl.cc:132: ASSERT(event->breakpoint->id() != 0);
named constant might make this clearer instead of zero.

[hausner, 2013-12-10 17:41:15]

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

[hausner, 2013-12-10 17:44:37]

Thank you.

https://codereview.chromium.org/110913002/diff/20001/runtime/bin/dbg_message.cc
File runtime/bin/dbg_message.cc (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/bin/dbg_message....
runtime/bin/dbg_message.cc:1077: }
On 2013/12/10 17:17:18, turnidge wrote:
> We could possibly use a named constant here instead of zero.  kNoBreakpoint or
> kBreakStepMode or something.

Done.

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1884: SourceBreakpoint* breakpoint) {
On 2013/12/10 17:17:18, turnidge wrote:
> Name is not consistent with 'bpt' used elsewhere in file.  Could consider
making
> them the same.

Done.

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.h#ne...
runtime/vm/debugger.h:272: Dart_Port isolate_id;
On 2013/12/10 17:17:18, turnidge wrote:
> At some point this may grow out of being a struct and may be better as a tree
of
> classes.  Not now, but something to consider.

I try not to be baroque. It's just a struct with a function that initializes the
fields automatically when declared.

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger_api_...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger_api_...
runtime/vm/debugger_api_impl.cc:132: ASSERT(event->breakpoint->id() != 0);
On 2013/12/10 17:17:18, turnidge wrote:
> named constant might make this clearer instead of zero.

Done.

[turnidge, 2013-12-10 18:55:23]

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/110913002/diff/20001/runtime/vm/debugger.h#ne...
runtime/vm/debugger.h:272: Dart_Port isolate_id;
On 2013/12/10 17:44:37, hausner wrote:
> On 2013/12/10 17:17:18, turnidge wrote:
> > At some point this may grow out of being a struct and may be better as a
tree
> of
> > classes.  Not now, but something to consider.
> 
> I try not to be baroque. It's just a struct with a function that initializes
the
> fields automatically when declared.

Yeah, agreed that being unnecessarily baroque is not good.

The counter argument (which we all know) goes like this:

- we need to maintain an explicit 'type' (instead of using C++ types)
- we need to sometimes use a field or not depending on the 'type', enforced by
convetion (instead of using subclassing, enforced by compiler)
- we probably have some dispatching code somewhere based on this 'type' (instead
of virtual dispatch)

Don't care for this review, though.