324775 - Merge expression classes

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The optimzed xpath implementation added some 8k of binary size on linux which raised a few complaints.

One way to reduce this overhead is to merge some of the similar classes we have. For example there is no point in have BooleanFunctionCall, NodesetFunctionCall, StringFunctionCall, and NumberFunctionCall as four separate classes. It'd be easier coding wise and smaller implementation wise to just have a single CoreFunctionCall.

The same can be done for the XSLT functions into XSLTFunctionCall.

Similarly we can merge AdditiveExpr and MultiplicativeExpr into NumberExpr. We could even make a common base class for NumberExpr, BooleanExpr and RelationExpr that takes care of the new optimization functions.

Are there more classes that lend themselfs to partial or full merger?

The only downside with this is that some of the source files might get bigger then we really want them. I especially fear a merged XSLTFunctionCall seing the size of txKeyFunctionCall and txFormatNumberFunctionCall.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This should do it. I merged the standard xpath functions into txXPathFunctionCall and three XSLT functions into txXSLTEnvironmentFunctionCall. I also merged txAdditiveExpr and txMultiplicativeExpr into txMathExpr.

I didn't give txMathExpr, txRelationalExpr and txBooleanExpr a common baseclass since there is farily little gain in it and it is kind'a ugly. Let me know if you think it's worth it.

I also broke out the requireParams calls from each |case| and instead put minimum and maximum param-count in the same table as the return type. See the beginning of txXPathFunctionCall::evaluate

Since all standard xpath functions now live in the same class it was unnecessary to have the long list of if-statements inside txExprParser, so got rid of that.


Other then that this patch only shuffels code around from the old classes to the new. I didn't put new license headers in there since this is basically just copying the old code. I ran buster on it and there were no regressions.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: includes all files

I forgot to cvs add txMathExpr. Also addresses one review comment.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 217069 [details] [diff] [review]
includes all files

>Index: src/xpath/txExpr.h
>===================================================================

>+    // This must be ordered in the same order as descriptTable in
>+    // txXPathFunctionCall.cpp. If you change one, change the other.
>+    enum eType {

>+        TX_BOOLEAN,        // boolean()
>+        TX_FALSE,          // false()
>+        TX_LANG,           // lang()
>+        TX_NOT,            // not()
>+        TX_TRUE            // true()

Let's drop the TX_ prefix, use _ if necessary.

>+    /*

/**

>+     * Creates a NodeSetFunctionCall of the given type

Update the comment?

>+     */
>+    txXPathFunctionCall(eType aType) : mType(aType)

txCoreFunctionCall?

>+class txMathExpr : public Expr {

>+    enum eOp { ADDITION, SUBTRACTION, DIVIDE, MULTIPLY, MODULUS };

ADD, SUBSTRACT

> 
>-     MultiplicativeExpr(nsAutoPtr<Expr>& aLeftExpr,
>-                        nsAutoPtr<Expr>& aRightExpr,
>-                        short aOp)
>-         : op(aOp),
>-           leftExpr(aLeftExpr),
>-           rightExpr(aRightExpr)
>+    txMathExpr(nsAutoPtr<Expr>& aLeftExpr,
>+               nsAutoPtr<Expr>& aRightExpr,
>+               eOp aOp)
>+        : mOp(aOp),
>+          mLeftExpr(aLeftExpr),
>+          mRightExpr(aRightExpr)

mOp needs to be last.

>+    nsAutoPtr<Expr> mLeftExpr, mRightExpr;
>+    eOp mOp;

>Index: src/xpath/txExprParser.cpp
>===================================================================

>@@ -228,22 +228,34 @@ nsresult

>+        case Token::DIVIDE_OP :
>+            expr = new txMathExpr(left, right,
>+                                          txMathExpr::DIVIDE);

Line this up correctly.

>+            break;
>+        case Token::MODULUS_OP :
>+            expr = new txMathExpr(left, right,
>+                                          txMathExpr::MODULUS);

Line this up correctly.

>+            break;
>+        case Token::MULTIPLY_OP :
>+            expr = new txMathExpr(left, right,
>+                                          txMathExpr::MULTIPLY);

Line this up correctly.

>Index: src/xpath/txXPathFunctionCall.cpp
>===================================================================

>+// This must be ordered in the same order as txXPathFunctionCall::eType.
>+// If you change one, change the other.
>+static txFunctionDescriptor descriptTable[] =

const?

>Index: src/xslt/txXSLTEnvironmentFunctionCall.cpp
>===================================================================

>+txXSLTEnvironmentFunctionCall::evaluate(txIEvalContext* aContext,
>+                                     txAExprResult** aResult)

Line this up correctly.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in, saved 4-5k