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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 1 16:23:24 PDT 2008

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

Attachment 22422: Fold only *, +, <<, >>, ~ operators (v3)

------- 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*,
+static ExpressionNode* makeAddNode(void*, ExpressionNode*, ExpressionNode*,
+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.

+"This test checks constant folding algorithm."

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

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.

More information about the webkit-reviews mailing list