[webkit-reviews] review granted: [Bug 127127] JSC: Adding UnlinkedCodeBlock::opDebugBytecodeOffsetForLineAndColumn(). : [Attachment 221490] patch for landing.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted  review:
Bug 127127: JSC: Adding
UnlinkedCodeBlock::opDebugBytecodeOffsetForLineAndColumn().
https://bugs.webkit.org/show_bug.cgi?id=127127

Attachment 221490: patch for landing.
https://bugs.webkit.org/attachment.cgi?id=221490&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221490&action=review


r=me, with two changes below:

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:387
> +    // Find the best fit op_debug opcode for the requested line and column:
> +    // The best fit op_debug will be the one that is preceding but is
nearest to
> +    // the specified line and column.
> +    LineColumnInfo::LineColumnPair target(line, column);
> +    LineColumnInfoList& infoList = opDebugLineColumnInfoList();
> +    int low = 0;
> +    int high = infoList.size();
> +    while (high - low > 1) {
> +	   int current = low + (high - low) / 2;
> +	   LineColumnInfo& currentInfo = infoList[current];
> +	   if (currentInfo == target) {
> +	       low = current;
> +	       break;
> +	   }
> +	   if (currentInfo < target)
> +	       low = current;
> +	   else
> +	       high = current;
>      }

Let's use std::lower_bound. Your understanding that it will "match only when
the element is not currently found in the sequence” seems to be out of date.

Specifically:
it = lower_bound()
if (target < *it)
    --it;
ASSERT(it >= begin && it < end && *it <= target)

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:408
> +    // Search backwards from the found op_debug to see if there are any
> +    // preceding op_debug bytecodes with the exact same line and column as
the
> +    // one we found. We want the earliest one because when we have multiple
> +    // potential breakpoint targets that map to a given line and column, a
> +    // debugger user would expect to break on the first one first and step
> +    // through the rest thereafter if needed.
> +    LineColumnInfo::LineColumnPair match(line, column);
> +    for (int i = low - 1; i >= 0; i--) {
> +	   LineColumnInfo& currentInfo = infoList[i];
> +	   if (currentInfo != match)
> +	       break;
> +	   low = i;
> +    }

Let's not do this. The Web Inspector doesn't set multiple breakpoints at the
exact same line/column position.

Instead, let's ASSERT:

    ASSERT(it == begin || *(it - 1) < *it);