[webkit-reviews] review granted: [Bug 19865] Sampling tool. : [Attachment 22454] New, cleaned-up implementation - no known bugs, no regressions.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 19865: Sampling tool.
https://bugs.webkit.org/show_bug.cgi?id=19865

Attachment 22454: New, cleaned-up implementation - no known bugs, no
regressions.
https://bugs.webkit.org/attachment.cgi?id=22454&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
This patch looks much improved. I have some minor comments, but I think you can
land the patch as-is, and address these comments in a separate patch.

MACHINE_SAMPLING_callingNativeFunction:

We've decided to use the word "host" rather than native to indicate functions
built into the library.

I think you only added this call to op_call. You need to add it to
op_construct, too.

I'm worried about what happens if somebody adds an opcode but forgets to update
the opcodeNames array. One way to prevent this would be to have a function with
a switch statement, instead of an array. That way, the compiler would warn if
not all the opcodes were covered by the switch. Another solution would be to
ASSERT somewhere that the number of elements in the array was equal to the
number of opcodes.

+	     for (ScopeSampleRecordMap::iterator iter =
m_scopeSampleMap->begin(); iter != m_scopeSampleMap->end(); ++iter)
+		 delete iter->second;

There's a helper function for this: deleteAllValues.

+	     delete m_scopeSampleMap;

There's a helper template for this: OwnPtr.

+ SAMPLING_TOOL_ENABLED

We have a specific style of macros -- USE, HAVE, etc. -- for #defines like
this. I think "ENABLE(SAMPLING_TOOL)" would be most appropriate in this case.
You can read more about these macros in wtf/Platform.h.

ScopeSampleRecord and SamplingTool should get their own files. Generally, we
try to put each class in its own file.

+	     ScopeSampleRecord* record =
m_scopeSampleMap->get(codeBlock->ownerNode);
+	     if (record)

You can move the declaration and assignment for "record" into the "if()". This
has the benefit of reducing the number of lines of code, and guaranteeing that
you cannot use the pointer if it is null (because it's only in scope if it is
not null).

+    const LineCountInfo *leftLineCount = reinterpret_cast<const LineCountInfo
*>(left);

Lines like this should put the "*" next to the type name, not one space away.