[Webkit-unassigned] [Bug 19408] Constant folding in parser

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=19408


cwzwarich at uwaterloo.ca changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #22422|review?                     |review-
               Flag|                            |




------- Comment #16 from cwzwarich at uwaterloo.ca  2008-08-01 16:23 PDT -------
(From update of attachment 22422)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.