Splitting debugger breakpoints into two parts

Splitting debugger breakpoints into two parts

Splitting breakpoints into SourceBreakpoint that represent
a user-defined source location of a breakpoint, and
CodeBreakpoint, which represents a code location. There can
be more than one CodeBreakpoint per SourceBreakpoint, e.g.
for functions that are also called as closures (and are thus
compiled twice.)

Functions are no longer compiled as a side effect of setting
a breakpoint. When they eventually get compiled, the previously
recorded SourceBreakpoint is found and a CodeBreakpoint is set.
Committed: https://code.google.com/p/dart/source/detail?r=4891

[hausner, 2012-03-02 17:46:02]

This review looks like a lot, but it is a lot of renaming and moving code to
different functions.

[regis, 2012-03-02 17:54:53]

LGTM

http://codereview.chromium.org/9581013/diff/2002/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

http://codereview.chromium.org/9581013/diff/2002/runtime/vm/debugger.h#newcode35
runtime/vm/debugger.h:35: bool IsEnabled() const { return is_enabled_; }
According to our coding style, it should be is_enabled(), since it is a simple
getter.

http://codereview.chromium.org/9581013/diff/2002/runtime/vm/debugger.h#newcode74
runtime/vm/debugger.h:74: bool IsEnabled() const { return is_enabled_; }
According to our coding style, it should be is_enabled(), since it is a simple
getter.

[hausner, 2012-03-02 18:01:19]

Thank you.

http://codereview.chromium.org/9581013/diff/2002/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

http://codereview.chromium.org/9581013/diff/2002/runtime/vm/debugger.h#newcode35
runtime/vm/debugger.h:35: bool IsEnabled() const { return is_enabled_; }
On 2012/03/02 17:54:53, regis wrote:
> According to our coding style, it should be is_enabled(), since it is a simple
> getter.
Forward ditto.

http://codereview.chromium.org/9581013/diff/2002/runtime/vm/debugger.h#newcode74
runtime/vm/debugger.h:74: bool IsEnabled() const { return is_enabled_; }
On 2012/03/02 17:54:53, regis wrote:
> According to our coding style, it should be is_enabled(), since it is a simple
> getter.
True. I anticipate though that this function could become more elaborate at some
point, involving a lookup or something. I would then have to rename is_enabled()
to IsEnabled(), which obliterates the whole point of having an accessor function
to shield callers from changes. I never understood that rule in our coding
standard. If you don't mind, I'll leave the name as is.