[webkit-reviews] review denied: [Bug 4698] kjs does not allow named functions in function expressions : [Attachment 3625] expanded patch implementing full ecma behavior for named FunctionExpr

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 4698: kjs does not allow named functions in function expressions
http://bugzilla.opendarwin.org/show_bug.cgi?id=4698

Attachment 3625: expanded patch implementing full ecma behavior for named
FunctionExpr
http://bugzilla.opendarwin.org/attachment.cgi?id=3625&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This patch looks great. Here are my comments:

A) There's no test case attached to this bug. We can't land this change without
a test.

B) Formatting-wise, the longer line in the yacc source file should follow the
approach used elsewhere -- putting the C code on a separate line so that the
left braces line up.

C) In FuncExprNode::evaluate:

+  bool named = (ident != Identifier::null());

should be:

    bool named = !ident.isNull();

D) For null pointers in new code, I prefer NULL over 0, and 0 over 0L. So we
definitely shouldn't initialize namedContainer to 0L.

E) I slightly prefer just plain "new ObjectImp" to "new ObjectImp()".

F) I think perhaps the member of FuncExprNode shoudl be named "name" rather
than "ident". Would we refer to this as the name of the function in the
expression or the identifier of the function?

G) The code that makes the property DontDelete unless the codeType is EvalCode
is confusing to me. Perhaps we could include a comment explaining that?