[webkit-reviews] review granted: [Bug 21123] using "arguments" in a function should not force creation of an activation object : [Attachment 23983] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>'s request for review:
Bug 21123: using "arguments" in a function should not force creation of an
activation object
https://bugs.webkit.org/show_bug.cgi?id=21123

Attachment 23983: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=23983&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	     m_jit.movl_mr(RegisterFile::OptionalCalleeActivation *
static_cast<int>(sizeof(Register)), X86::edi, X86::eax);
+	     m_jit.orl_mr(RegisterFile::OptionalCalleeArguments *
static_cast<int>(sizeof(Register)), X86::edi, X86::eax);

I'm disappointed that we have to make the fast case slower here by examining
both of these. I wonder if some day we can find a way to make it faster.

+    } else if (JSValue* arguments =
r[RegisterFile::OptionalCalleeArguments].getJSValue()) {
+	 if (arguments->isObject(&Arguments::info))
+	     static_cast<Arguments*>(arguments)->copyRegisters();
+    }

Why is this isObject check needed? When does OptionalCalleeArguments contains
something that's non-zero but is not an Arguments object?

+					 { $$ =
createNodeFeatureInfo<PropertyNode*>(makeGetterOrSetterPropertyNode(globalPtr,
*$1, *$2, $4.m_node.head, $7, LEXER->sourceRange($6, $8)), $4.m_featureInfo |
ClosureFeature, 0); DBG($7, @6, @8); $7->setUsesArguments($7->usesArguments() |
($4.m_featureInfo & ArgumentsFeature)); if (!$$.m_node) YYABORT; }

I'd expect to have the new code be before the DBG rather than after. As these
lines of code get super-long, I'd interested in having some of this logic move
into a helper function so it's not all on one giant source line in the grammar.
Or some other way of formatting this with multiple lines so it's easier to
read.

This is a great speedup!

r=me