[webkit-reviews] review granted: [Bug 16471] Completions need to be smaller (or not exist at all) : [Attachment 18003] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 16471: Completions need to be smaller (or not exist at all)
http://bugs.webkit.org/show_bug.cgi?id=16471

Attachment 18003: patch
http://bugs.webkit.org/attachment.cgi?id=18003&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
Ok, first of all, this is an absolutely wonderful change!  2.4%, wow.

I have two nits:

First,
CaseClauseNode::executeStatements:
seems strange that that returns jsUndefined() instead of 0 (like other excute
statements).  I didn't follow the logic all the way through to see if
jsUndefined was necessary in this case.


Second,
I dislike exposing of setCompletionType.  I think that doing so is error prone,
and leads to less-clean code.

I like your use of setErrorCompletion and setBreakCompletion and would suggest
changing the expected for *all* completion returns to:

return setNormalCompletion(ret);
return setNormalCompletion();
return setThrowCompletion(val);
return setInteruptedCompletion();

That would basically make setCompletionType an "implementation detail" and in
all cases (except perhaps due to try/catch/finally) could be a private method
on ExecState.  It would also turn current 2-line statements of:
setInteruptedCompletion()
return 0;

into a single line:
return setInterupedCompletion();


Oh, and also:
I kinda laughed at "continueDoLoop", I probably would have named it
doWhileLoop. :)

Oh, and if you change to use the "return setNormalCompletion();" idiom, you
would end up make hitStatement() not alter the completion type directly. 
(which looks kinda odd to me currently).

Great patch.  Landable as is.  I'd prefer to see it change to the return
set*Completion(); idiom.