[webkit-reviews] review denied: [Bug 19408] Constant folding in parser : [Attachment 22422] Fold only *, +, <<, >>, ~ operators (v3)

[bugzilla-daemon at webkit.org]

Cameron Zwarich (cpst) <cwzwarich at uwaterloo.ca> has denied Gabor Loki
<loki at inf.u-szeged.hu>'s request for review:
Bug 19408: Constant folding in parser
https://bugs.webkit.org/show_bug.cgi?id=19408

Attachment 22422: Fold only *, +, <<, >>, ~ operators (v3)
https://bugs.webkit.org/attachment.cgi?id=22422&action=edit

------- Additional Comments from Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>
As Geoff mentions, you should probably see what happens when you do constant
folding for division. Why don't you fold the other bitwise operations.

+static ExpressionNode* makeMultNode(void*, ExpressionNode*, ExpressionNode*,
bool);
+static ExpressionNode* makeAddNode(void*, ExpressionNode*, ExpressionNode*,
bool);
+static ExpressionNode* makeLeftShiftNode(void*, ExpressionNode*,
ExpressionNode*, bool);
+static ExpressionNode* makeRightShiftNode(void*, ExpressionNode*,
ExpressionNode*, bool);

The last bool argument should probably be named here, because it isn't clear
what it means. It should also be given a descriptive name in the functions
themselves, at least something better than "ra".

+    if (m1->isNumber() && m2->isNumber())
+	 return makeNumberNode(globalPtr, static_cast<NumberNode*>(m1)->value()
* static_cast<NumberNode*>(m2)->value());
+    else
+	 return new MultNode(GLOBAL_DATA, m1, m2, ra);

In WebKit, we don't return from an else like this, we simply remove the else
and put the return statement after the if statement.

+description(
+"This test checks constant folding algorithm."
+);

This might be better put as "This test checks the correctness of constant
folding."

We tend to prefer 4 space indentation in JavaScript tests, but why did you make
your own shouldBe() function? I don't see any reason why you can't use the
standard one.

Other than that, it seems like a good patch. I apologize on behalf of the
JavaScript team for not reviewing your patch earlier. Most of the development
takes place on IRC, so sometimes we are not as responsive to one-off patches
posted on the Bugzilla as we should be. I will try to ensure that this doesn't
happen in the future.