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

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

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




------- Comment #5 from darin at apple.com  2008-06-12 07:21 PDT -------
(From update of attachment 21658)
This looks great!

The patch to the ChangeLog file has tabs in it; we can't check in files with
tabs so it's better for the committer if you author the file without tab
characters.

For new files in JavaScriptCore we're using naming more like the ones in
WebCore. So it should be ConstantFolding.h rather than const_folding.h.

+ ExpressionNode* foldConst(enum ConstFoldOperType op, ExpressionNode* m1,
ExpressionNode* m2)

No need for "enum" here.

+             UString s1 = (static_cast<StringNode*>(m1))->value();
+             UString s2 = (static_cast<StringNode*>(m2))->value();

+             d1 = (static_cast<NumberNode*>(m1))->value();

+             d1 = (static_cast<BooleanNode*>(m1))->value();


These expressions have extra unneeded parentheses.

+     int n1 = 0, n2 = 0;

These variables seem to be booleans, but you are using int instead of bool.
Also, these declarations should be just before the values are used.

+             if (op == operatorUnaryLogicalNot) {
+                 if (sn->value().isEmpty())
+                     return new TrueNode;
+                 else
+                     return new FalseNode;
+             } else if (op != operatorBinaryAdd) {
+                 d1 = sn->value().toDouble();
+                 if (!isnan(d1))
+                     n1 = 1;
+             }

We normally do not put an else after a return (done here twice).

Also, the TrueNode/FalseNode idiom happens so many times I think we should add
a helper function that does that to avoid all those different if statements.

+                 return new NumberNode(JSValue::toInt32(d1) <<
((uint32_t)JSValue::toUInt32(d2) & 0x1f));

The typecast of the result of toUInt32 seems unneeded.

+         if (n2)
+             switch(op) {

There should be braces after "if (n2)". We only omit the braces if the body is
one line -- one statement is not enough.

There should be a space after "switch".

+ #ifndef CONST_FOLDING_H_

Style guide says this should be the same case as the filename. If you use
ConstantFolding.h, then it should be ConstantFolding_h.

 447   | '+' UnaryExpr                       { ExpressionNode *en;
 448                                           if ((en =
foldConst(operatorUnaryPlus, $2.m_node, 0)))
 449                                             $$ =
createNodeFeatureInfo<ExpressionNode*>(en, $2.m_featureInfo);
 450                                           else
 451                                             $$ =
createNodeFeatureInfo<ExpressionNode*>(new UnaryPlusNode($2.m_node),
$2.m_featureInfo); }

These would read better if you declared the ExpressionNode* inside the if
statement. The * goes next to the type name, so it should be "ExpressionNode*",
not "ExpressionNode *".

I don't think it's great to add isBoolean() and BooleanNode. This just makes
too many classes. There could be separate isTrue and isFalse. But the reason
true and false have separate nodes in the first place is from when the tree was
used for execution. Nowadays it would be fine to eliminate TrueNode and
FalseNode altogether and just have a BooleanNode.

 356         virtual bool isString() const KJS_FAST_CALL { return true; }

This might sound surprising, but the override should be private. The reason for
that is that it's a programming mistake to call isString() if you know it's a
string node, and by making the function private you get a compiler error if you
make that error.

We need to add a test along with this code change. It can go in
LayoutTests/fast/js. We need a test case for each constant folding case, to
make sure we're actually exercising the folded constant code. I should be able
to make the test fail by changing anything in the constant folding function.

I'm tempted to say review+ because all the comments are minor style issues. But
for now I'll say review- because we really need a test case.

This really is great work. I'm looking forward to getting it in.


-- 
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.