[webkit-reviews] review denied: [Bug 19408] Constant folding in parser : [Attachment 21527] Const.Folding.v1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 9 06:48:22 PDT 2008

Darin Adler <darin at apple.com> has denied Gabor Loki <loki at inf.u-szeged.hu>'s
request for review:
Bug 19408: Constant folding in parser

Attachment 21527: Const.Folding.v1

------- Additional Comments from Darin Adler <darin at apple.com>
The general approach looks OK.

Here's a quick pass of review.

+    OP_UN_PLUS,	 /* '+'   */

For an enum like this, there's no reason to use the ALL_CAPS style. That's
usually reserved for macros, where you're concerned about conflict with other
identifiers, ignoring namespaces. You should also abbreviate less. I think:


is just fine, and OP_UN_PLUS is not really better.

+static ExpressionNode* foldConst(char, ExpressionNode*, ExpressionNode*);

"char" is not the right type to use here. You should give the enum type a name
and use that type. Even if you did want to use a type in the "char" family, it
would probably be unsigned char. Since "char" is signed or unsigned depending
on the compiler.

+					   if ((en = foldConst(OP_UN_LNOT,
$2.m_node, NULL)))

We use "0" in JavaScriptCore, not NULL.

+	 if (typeid(*m1) == typeid(StringNode) && typeid(*m2) ==
typeid(StringNode)) {

There's no RTTI in WebCore so you can't use typeid. If you want to find the
type of a node, you'll need to add an isStringNode() function. But that's a
good thing, because a virtual function call is faster than a typeid check on
most compilers.

+	     UString s1 = ((StringNode*)m1)->value();

We use C++ style casts, such as static_cast, not C style casts like this.

+static ExpressionNode* foldConst(char op, ExpressionNode* m1, ExpressionNode*

I think it would be best to put these functions in a different file rather than
at the bottom of the grammar.

It's critical to use SunSpider and see how this constant folding affects speed
on the benchmark.

More information about the webkit-reviews mailing list