[webkit-reviews] review granted: [Bug 18774] SQUIRRELFISH: print meaningful error messages : [Attachment 22359] Patch o' doom

[bugzilla-daemon at webkit.org]

Cameron Zwarich (cpst) <cwzwarich at uwaterloo.ca> has granted Oliver Hunt
<oliver at apple.com>'s request for review:
Bug 18774: SQUIRRELFISH: print meaningful error messages
https://bugs.webkit.org/show_bug.cgi?id=18774

Attachment 22359: Patch o' doom
https://bugs.webkit.org/attachment.cgi?id=22359&action=edit

------- Additional Comments from Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>
+int CodeBlock::lineNumberForVPC(const Instruction* vPC) {

The opening brace should be on the next line.

+	 * kjs/SourceRange.h:

This is in the wrong position of the header list.

+		 divot = 0; // Overflow has occurred, we can only give line
number info for errors for this region

Might as well put the comment on its own line(s) like all the others in this
if-else statement.

+    JSObject* exception = Error::create(exec, TypeError,
createErrorMessage(exec, codeBlock, line, divotPoint, divotPoint + endOffset,
value, message), line, codeBlock->ownerNode->sourceId(),
codeBlock->ownerNode->sourceURL());

Maybe it would be better to break off the createErrorMessage() call into its
own line, because this line is already quite long. This goes for the other
creations of errors as well.

+	 int32_t m_startOffset;
+	 

I don't think you want the extra line here.

Other than that, it looks good, so r=me with those changes. Do you have any
tests for your new exception messages and line/column info?