Debug: support breakpoints set in the middle of statement

Debug: support breakpoints set in the middle of statement

R=yangguo@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=15416

[Peter.Rybin, 2013-06-13 12:12:02]

On 2013/06/09 22:23:18, peter.rybin wrote:

ping

[Peter.Rybin, 2013-06-24 17:12:59]

On 2013/06/13 12:12:02, peter.rybin wrote:
> On 2013/06/09 22:23:18, peter.rybin wrote:
> 
> ping

ping

[Yang, 2013-06-25 11:32:00]

Looking good so far. Got a few comments. Also, test case would be great.

Again, sorry about being so slow on code reviews!

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:280: other_script.id, this.line_, this.column_,
this.groupId_, this.statement_aligned_);
80 char limit

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:447: var actual_position = %SetScriptBreakPoint(script,
position, this.statement_aligned_, break_point);
80 char limit

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:810: opt_condition, opt_groupId, opt_statement_aligned) {
80 char limit

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:813: opt_condition, opt_groupId, opt_statement_aligned);
80 char limit

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc#newcode239
src/debug.cc:239: bool statement_alighned) {
Change variable name to "statement_aligned".

Suggestion: I'd prefer introducing an enum for better redability. Something like

enum PositionAlignment {
  STATEMENT_ALIGNED = 0,
  BREAK_POSITION_ALIGNED = 1
};

The advantage is that the use sites would read like

it.FindBreakLocationFromPosition(
  *source_position, STATEMENT_ALIGNED);

Of course you would have to do that in javascript as well. You could add those
enum values as constants to macros.py.

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc#newcode247
src/debug.cc:247: if (statement_alighned) {
You could use a ternary operator to simplify this
int next_position = statement_aligned ? this->statement_position()
                                      : this->position();

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc#newcode1199
src/debug.cc:1199: bool statement_alighned) {
Ditto.

[Peter.Rybin, 2013-06-27 14:07:29]

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:280: other_script.id, this.line_, this.column_,
this.groupId_, this.statement_aligned_);
On 2013/06/25 11:32:00, Yang wrote:
> 80 char limit

Done.

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:447: var actual_position = %SetScriptBreakPoint(script,
position, this.statement_aligned_, break_point);
On 2013/06/25 11:32:00, Yang wrote:
> 80 char limit

Done.

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:810: opt_condition, opt_groupId, opt_statement_aligned) {
On 2013/06/25 11:32:00, Yang wrote:
> 80 char limit

Done.

https://codereview.chromium.org/16093040/diff/1007/src/debug-debugger.js#newc...
src/debug-debugger.js:813: opt_condition, opt_groupId, opt_statement_aligned);
On 2013/06/25 11:32:00, Yang wrote:
> 80 char limit

Done.

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc#newcode239
src/debug.cc:239: bool statement_alighned) {
On 2013/06/25 11:32:00, Yang wrote:
> Change variable name to "statement_aligned".
> 
> Suggestion: I'd prefer introducing an enum for better redability. Something
like
> 
> enum PositionAlignment {
>   STATEMENT_ALIGNED = 0,
>   BREAK_POSITION_ALIGNED = 1
> };
> 
> The advantage is that the use sites would read like
> 
> it.FindBreakLocationFromPosition(
>   *source_position, STATEMENT_ALIGNED);
> 
> Of course you would have to do that in javascript as well. You could add those
> enum values as constants to macros.py.

I'm fine with this idea. I did JavaScript part as a regular object, like it is
done in debug-debugger.js

Done

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc#newcode247
src/debug.cc:247: if (statement_alighned) {
On 2013/06/25 11:32:00, Yang wrote:
> You could use a ternary operator to simplify this
> int next_position = statement_aligned ? this->statement_position()
>                                       : this->position();

I guess with enum I should do switch? Or is it too long?

https://codereview.chromium.org/16093040/diff/1007/src/debug.cc#newcode1199
src/debug.cc:1199: bool statement_alighned) {
On 2013/06/25 11:32:00, Yang wrote:
> Ditto.

Done.

[Peter.Rybin, 2013-06-27 14:08:50]

> Looking good so far. Got a few comments. Also, test case would be great.

I added a test case. It required to extend runtime.cc a bit more.

> Again, sorry about being so slow on code reviews!
It's ok, I had a many other tasks to program.

[Peter.Rybin, 2013-07-01 10:45:05]

Let's finish this off :)

[Yang, 2013-07-01 12:11:04]

On 2013/07/01 10:45:05, peter.rybin wrote:
> Let's finish this off :)

LGTM!

[Peter.Rybin, 2013-07-01 12:54:24]

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