[webkit-reviews] review granted: [Bug 127127] JSC: Adding UnlinkedCodeBlock::opDebugBytecodeOffsetForLineAndColumn(). : [Attachment 221402] the patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 127127: JSC: Adding
UnlinkedCodeBlock::opDebugBytecodeOffsetForLineAndColumn().
https://bugs.webkit.org/show_bug.cgi?id=127127

Attachment 221402: the patch.
https://bugs.webkit.org/attachment.cgi?id=221402&action=review

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


r=me

Please fix the build, and a few issues below.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2578
> +    if (bytecodeOffset != UINT_MAX) {

Please use WTF::notFound.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:260
> +inline void
UnlinkedCodeBlock::decodeExpressionRangeLineAndColumn(ExpressionRangeInfo&
info,

Let's call this "getLineAndColumn".

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:283
> +// dumpExpressionRangeInfo() and dumpLineColumnInfoList() are utility
functions used for
> +// debugging only. They are not normally called, but we include them in the
debug build
> +// by default to avoid bit-rot.

I don't think this comment adds anything.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:371
> +	   return UINT_MAX;

Please use WTF::notFound.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:-307
> +    // 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;
> +	   else
> +	       high = current;
>      }
> -    } // switch

This looks like a binary chop algorithm. Did you consider WTF::binarySearch or
std::binary_search?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:396
> +    // Find the earliest op_debug opcode that has this same best fit line
and column:

It would be better to comment here *why* you're looking for the earliest item.
You explained that in your ChageLog, but not in your code.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:431
> +    m_rareData->m_opDebugLineColumnInfoList =
std::unique_ptr<LineColumnInfoList>(new LineColumnInfoList());

Please use make_unique.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:559
>	   Vector<ExpressionRangeInfo::FatPosition>
m_expressionInfoFatPositions;
> +	   std::unique_ptr<LineColumnInfoList> m_opDebugLineColumnInfoList;

Why is m_opDebugLineColumnInfoList a pointer to vector, while
m_expressionInfoFatPositions is an inline vector? Should they both be pointers
to vectors?