210528 - modernize expressions

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

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

This patch breaks out some of the common parts from bug 208172 and bug 206445 so
that we can get a quick review so that both bugs can move on. Basically it makes
us use nsAutoPtrs where appropriate in expressions and properly 'm' and 'a'
prefix members and arguments.

It also makes some of the ::addXXX functions return nsresults so that we can do
proper OOM checking. I did not however add code to actually check the
returnvalues since that is covered by bug 206445

Comment 1

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

Attachment: patch to fix

Comment 2

[Axel Hecht]

Comment on attachment 126384 [details] [diff] [review]
patch to fix

please fix the
/**
 **/
and
/*
 */
comments in the cpp, too.
And there are 11 .cpp and one .h missing modelines.

There is at least 
PRBool FunctionCall::requireParams
without the return type on a separate line.
No need to touch those in ExprParser, ExprLexer, though, as those are almost
fixed and would just give more merge conflicts.
+void FilterExpr::toString
and LocationStep, guess you gotta grep your tree for this.
Change List into txList, too.
I'd like to see a new patch to check if I find something that I forgot.

Comment 3

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

Attachment: smaller patch

This one only does what is neccesary, i.e. convert members/ctor-arguments to be
nsAutoPtrs as well as remove empty ctors/dtors

Comment 4

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

Comment on attachment 126473 [details] [diff] [review]
smaller patch

sorry, missed a couple of things

Comment 5

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

Attachment: this one is good

same as before, but this one fixes all the addXXX functions appropriatly

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 126474 [details] [diff] [review]
this one is good

The only thing that I'm concerned about with this patch is the passing of
nsAutoPtr<>'s by value, that will most likely cause generation of code you
could avoid if you'd pass them by reference, and thus it would probably be
faster (though not necessarily measurably) if you'd use references.

sr=jst either way tho.

Comment 7

[Axel Hecht]

Comment on attachment 126474 [details] [diff] [review]
this one is good

-RelationalExpr::RelationalExpr(Expr* aLeftExpr, Expr* aRightExpr,
+RelationalExpr::RelationalExpr(nsAutoPtr<Expr> aLeftExpr,
+				nsAutoPtr<Expr> aRightExpr,
				RelationalExprType aOp)
     : mLeftExpr(aLeftExpr), mRightExpr(aRightExpr), mOp(aOp)

separate lines for the initialisers? Guess you're close enough to that line ;-)

with that, r=me.
I'd like to keep the pass-by-value. From lookin at the code, it should have
small to no impact, and you can call into the functions with both regular
pointers and nsAutoPtrs. We can still change those to references later, if we
think that it really matters.

Comment 8

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

checked in

Comment 9

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

backed out due to ports bustage

Comment 10

[Axel Hecht]

not just ports, but obviously gcc3.3 is bailing on

void doStuff(nsAutoPtr<Expr> aExpr);

Expr* foo = bar();
doStuff(foo);

Sad, this was supposed to be the good thing about it. So it'll be only huge 
patches and pass by reference, it seems.

Comment 11

[jag (Peter Annema)]

Note that most auto_ptr implementations have |explicit auto_ptr(T* aPtr = 0);|,
prohibiting this automatic conversion, probably to avoid surprises.

Even without the explicit (as in our code), this won't work because of
|nsAutoPtr(nsAutoPtr<T>& aPtr);|. The compiler can construct a temporary
nsAutoPtr<Expr> from an Expr* through |nsAutoPtr(T*);|, which then has to be
copied into the function argument through nsAutoPtr's copy constructor. This
step fails, since the copy constructor is non-const, and temporary objects can
only be bound to const references. That's also why |nsAutoPtr<A> a(new A());|
works and |nsAutoPtr<A> a = new A();| doesn't.

Comment 12

[Axel Hecht]

Comment on attachment 126474 [details] [diff] [review]
this one is good

this one got backed out

Comment 13

[Axel Hecht]

Attachment: make lots of arguments nsAutoPtr<>&

As it seems, we can do this step by step.
Here is the "convert most of xpath to nsAutoPtr&" patch.
For a standalone opt build, it actually reduces binary size. I didn't see
any impact off the inlined destructors on binary size.

Comment 14

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

Comment on attachment 129375 [details] [diff] [review]
make lots of arguments nsAutoPtr<>&

>Index: xpath/Expr.h
>@@ -348,7 +348,11 @@
>      * @param nodeExpr the NodeExpr to use when matching Nodes
>      * @param axisIdentifier the Axis Identifier in which to search for nodes
>     **/
>-    LocationStep(nsAutoPtr<txNodeTest> aNodeTest, LocationStepType aAxisIdentifier);
>+    LocationStep(nsAutoPtr<txNodeTest>& aNodeTest,
>+                 LocationStepType aAxisIdentifier);
>+    ~LocationStep()
>+    {
>+    }

Why the empty dtor? Just using the default one should produce the exact same
result. Same applies to most other classes in Expr.h

>+     AdditiveExpr(nsAutoPtr<Expr>& aLeftExpr, nsAutoPtr<Expr>& aRightExpr,
>+                  short aOp);
>+    ~AdditiveExpr()

Indentation

>+     BooleanExpr(nsAutoPtr<Expr>& aLeftExpr, nsAutoPtr<Expr>& aRightExpr,
>+                 short aOp);
>+    ~BooleanExpr()

Indentation

>+     MultiplicativeExpr(nsAutoPtr<Expr>& aLeftExpr,
>+                        nsAutoPtr<Expr>& aRightExpr,
>+                        short aOp);
>+    ~MultiplicativeExpr()

Indentation

>Index: xpath/ExprParser.cpp
>+                nsAutoPtr<Expr> binary;
>                 while (!exprs.isEmpty() &&
>                         precedenceLevel(tok->type) 
>                        <= precedenceLevel(((Token*)ops.peek())->type)) {
>-                    expr = createBinaryExpr((Expr*)exprs.pop(),
>-                                             expr,
>-                                             (Token*)ops.pop());
>+                    nsAutoPtr<Expr> left(NS_STATIC_CAST(Expr*, exprs.pop()));
>+                    binary = createBinaryExpr(left, expr, (Token*)ops.pop());
>+                    expr = binary;

doing |expr = createBinaryExpr(...| will work just fine. The rhs expression is
always fully evaluated before the operator= is executed.

>     while (!exprs.isEmpty()) {
>-        expr = createBinaryExpr((Expr*)exprs.pop(), expr, (Token*)ops.pop());
>+        nsAutoPtr<Expr> left(NS_STATIC_CAST(Expr*, exprs.pop()));
>+        nsAutoPtr<Expr> binary;
>+        binary = createBinaryExpr(left, expr, (Token*)ops.pop());
>+        expr = binary;

same here.

>@@ -372,22 +377,20 @@
> 
>     if (lexer.peek()->type == Token::L_BRACKET) {
> 
>-        FilterExpr* filterExpr = new FilterExpr(expr);
>+        nsAutoPtr<FilterExpr> filterExpr(new FilterExpr(expr));
>         if (!filterExpr) {
>             // XXX ErrorReport: out of memory
>-            delete expr;
>             return 0;
>         }
> 
>         //-- handle predicates
>         if (!parsePredicates(filterExpr, lexer, aContext)) {
>-            delete filterExpr;
>             return 0;
>         }
>-        expr = filterExpr;
>+        expr = filterExpr.forget();

No need to call .forget()

>@@ -831,17 +832,17 @@
> **/
> Expr* ExprParser::createUnionExpr(ExprLexer& lexer, txIParseContext* aContext)
> {
>-    Expr* expr = createPathExpr(lexer, aContext);
>+    nsAutoPtr<Expr> expr;
>+    expr = createPathExpr(lexer, aContext);

nsAutoPtr<Expr> expr(createPathExpr(lexer, aContext));

>@@ -913,7 +913,8 @@
>         //-- eat Token
>         lexer.nextToken();
> 
>-        Expr* expr = createExpr(lexer, aContext);
>+        nsAutoPtr<Expr> expr;
>+        expr = createExpr(lexer, aContext);

Same here

>@@ -953,14 +954,14 @@
>     }
> 
>     while (1) {
>-        Expr* expr = createExpr(lexer, aContext);
>+        nsAutoPtr<Expr> expr;
>+        expr = createExpr(lexer, aContext);

And here

>Index: xpath/MultiplicativeExpr.cpp
>-MultiplicativeExpr::MultiplicativeExpr(Expr* leftExpr, Expr* rightExpr, short op) {
>-    this->op = op;
>-    this->leftExpr = leftExpr;
>-    this->rightExpr = rightExpr;
>-} //-- MultiplicativeExpr
>+MultiplicativeExpr::MultiplicativeExpr(nsAutoPtr<Expr>& aLeftExpr,
>+				       nsAutoPtr<Expr>& aRightExpr,
>+				       short aOp)

Indentation

>Index: xpath/PathExpr.cpp
>@@ -56,8 +56,7 @@
> {
>     txListIterator iter(&expressions);
>     while (iter.hasNext()) {
>-         PathExprItem* pxi = (PathExprItem*)iter.next();
>-         delete pxi->expr;
>+         PathExprItem* pxi = NS_STATIC_CAST(PathExprItem*, iter.next());

just do |delete NS_STATIC_CAST(...|


With that, r=sicking

Comment 15

[Axel Hecht]

> Why the empty dtor? Just using the default one should produce the exact same
> result. Same applies to most other classes in Expr.h

Warning fix. Overloading the virtual destructor with the default one is worth 
a warning for some kind of gcc. Just trying to not have timeless do that.

> doing |expr = createBinaryExpr(...| will work just fine. The rhs expression is
> always fully evaluated before the operator= is executed.

This is true now, but is false once we have the nsresult return values. This
should keep down my merge conflicts.

> >-        expr = filterExpr;
> >+        expr = filterExpr.forget();
>
> No need to call .forget()

filterExpr is a nsAutoPtr<FilterExpr>, expr is a nsAutoPtr<Expr>. The assignment
operator only kicks in for nsAutoPtrs of the same class, not inherited stuff.
Thus I need to handle that explicitly. Tried and failed to do otherwise.

> just do |delete NS_STATIC_CAST(...|

I may just skip that chunk alltogether, and leave the casts to the prettyfyin
patch

Comment 16

[Axel Hecht]

Attachment: new patch

This is the new patch.
I changed the delete NS_STATIC thing in the destructor, and the indention
stuff.
All other comments have been addressed by my latest comment.
(apart from the "put the createFoo into the nsAutoPtr constructor, which is in
preparation for the nsresult bug, too.)

Comment 17

[Axel Hecht]

Comment on attachment 129939 [details] [diff] [review]
new patch

re-asking reviews. The only critical thing would be whether we want to have
those explicit destructors. I added them for bug 206115, which may or may not
be a good reason.

Comment 18

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

Comment on attachment 129939 [details] [diff] [review]
new patch

I still think that we shouldn't have the empty dtors and looking at bug 206115
convinces me even more. According to that bug there first of all hasn't been a
real decision on what to do, second the current suggestion involves using #if's
to only use them sometimes.

So IMHO we should wait until a final verdict (and possibly a macro) before
starting to add code.

>Index: xpath/RelationalExpr.cpp
>-RelationalExpr::RelationalExpr(Expr* aLeftExpr, Expr* aRightExpr,
>+RelationalExpr::RelationalExpr(nsAutoPtr<Expr>& aLeftExpr,
>+                               nsAutoPtr<Expr>& aRightExpr,
>                                RelationalExprType aOp)
>     : mLeftExpr(aLeftExpr), mRightExpr(aRightExpr), mOp(aOp)
> {
> }
>+
> 
> /**
>  *  Compares the two ExprResults based on XPath 1.0 Recommendation (section 3.4)

Why add the newline? No biggie though.

>Index: xpath/UnaryExpr.cpp
>-UnaryExpr::~UnaryExpr()
>+UnaryExpr::UnaryExpr(nsAutoPtr<Expr>& aExpr)
>+  : expr(aExpr)
> {
>-    delete expr;
> }
>+

Same here

>Index: xpath/UnionExpr.cpp
>-void UnionExpr::addExpr(Expr* expr) {
>-    if (expr)
>-      expressions.add(expr);
>+nsresult
>+UnionExpr::addExpr(nsAutoPtr<Expr>& expr) {
>+    nsresult rv = expressions.add(expr);
>+    if (NS_SUCCEEDED(rv)) {
>+        expr.forget();
>+    }
>+    return rv;
> } //-- addExpr

Please fix the '{' around the functionbody.

With that r=sicking

Comment 19

[Axel Hecht]

Comment on attachment 129939 [details] [diff] [review]
new patch

carrying r=sicking.
I removed the destructors and did the whitespace, {

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 129939 [details] [diff] [review]
new patch

>Index: xpath/AdditiveExpr.cpp
>===================================================================

>+AdditiveExpr::AdditiveExpr(nsAutoPtr<Expr>& aLeftExpr,
>+			   nsAutoPtr<Expr>& aRightExpr, short aOp)
>+  : op(aOp),
>+    leftExpr(aLeftExpr),
>+    rightExpr(aRightExpr)

While you're at it, could you make the members prefixed with m*?

>Index: xpath/Expr.h
>===================================================================

>@@ -119,7 +119,7 @@
>      * Adds the given parameter to this FunctionCall's parameter list
>      * @param expr the Expr to add to this FunctionCall's parameter list
>     **/
>-    nsresult addParam(Expr* aExpr);
>+    nsresult addParam(nsAutoPtr<Expr>& aExpr);

I still prefer regular pointers for normal functions and .forget() in the
caller (exception for constructors).

>Index: xpath/ExprParser.cpp
>===================================================================

>+Expr*
>+ExprParser::createBinaryExpr(nsAutoPtr<Expr>& left, nsAutoPtr<Expr>& right,
>+                             Token* op)

aLeft, aRight, aOp?

>+nsresult
>+UnionExpr::addExpr(nsAutoPtr<Expr>& expr) {

Move the brace one line down.

Please add another patch (if you agree with some of the comments) and I'll have
a last look when I'm more awake.

Comment 21

[Axel Hecht]

> While you're at it, could you make the members prefixed with m*?
remember, I wanted to that in a separate "style only" patch. Same for the aFoo
comment.

> I still prefer regular pointers for normal functions and .forget() in the
> caller (exception for constructors).
I still think that having nsAutoPtrs (reference or not) in the signature is a 
good idea, as it indicates passing the ownership.

Gonna do the brace stuff.

Comment 22

[Peter Van der Beken [:peterv]]

> I still think that having nsAutoPtrs (reference or not) in the signature is a 
> good idea, as it indicates passing the ownership.

Passing nsAutoPtr by reference does not signal transfer of ownership, you have
to look in the implementation to know that it does. At least .forget() signals
releasing the ownership in the caller, which is the important part.

Comment 23

[Axel Hecht]

Comment on attachment 129939 [details] [diff] [review]
new patch

obsoleting. Peter and I decided to do the prettify stuff first. Then we can
start the fight over whether we want/need/should have nsAutoPtr signatures
again. There are pros and cons for both schemes.

pro nsAutoPtr&:
signature tells ownership and works thru more than one call depth (foo calls
bar calls baz)

pro regular pointer:
using smartPtr.forget() indicates the nulling of the pointer at the point of
the call itself.

Once we have those files in shape, it will hopefully become apparent how often
we stuff that aFoo right into a nsAutoPtr. That would be ugly and might make
the day to optimize that into passing nsAutoPtr&.

Comment 24

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

anything left to do here?

Comment 25

[Axel Hecht]

yes, comments, member names, indenting, stuff like that.
Things that would normally clutter other patches.

Comment 26

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

reassining bugs i'm not working on to default owner

Comment 27

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

Attachment: Use nsTArray and remove nsAutoPtr arguments.

This patch does a few things things:

1. Use an owning nsTArray rather than txList to store sub-expressions and
   sub-patterns.

2. Remove nsAutoPtr ctor arguments and use normal raw pointers instead.

3. Inline simple expression/pattern methods and remove empty ctors/dtors.
   Doesn't inline virtual methods that are only called virtually anyway.

4. Replace txPattern::getSimplePatterns with calls to txPattern::getType and
   txPattern::getSubPatternAt/setSubPatternAt


for 1 I created a txOwningArray that subclass nsTPtrArray. An alternative is to use nsTArray<nsAutoPtr<X> >. The difference is that nsTArray<nsAutoPtr> deletes when replacing and removing existing elements, something we for the most part don't want. Also nsTArray<nsAutoPtr>::SafeElementAt is more cumbersome to use than nsTPtrArray<>::SafeElementAt.

Comment 28

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

Oh, I also did

5. Change the weird ownership model of UnionExpr::addExpr and similar functions.

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 247792 [details] [diff] [review]
Use nsTArray and remove nsAutoPtr arguments.

Looks great. I hope I didn't miss anything, changing ownership transfer is tricky :-/.

>Index: src/base/txOwningArray.h
>===================================================================

>+class txOwningArray : public nsTPtrArray<E> {

Nit: move the brace one line down.

>+    ~txOwningArray() {

Nit: move the brace one line down.

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

>+    txLiteralExpr(double aDbl)
>+        : mValue(new NumberResult(aDbl, nsnull))
>+    {
>+    }
>+    txLiteralExpr(const nsAString& aStr)
>+        : mValue(new StringResult(aStr, nsnull))
>+    {
>+    }

We need to handle out-of-memory for these.

>+     BooleanExpr(Expr* aLeftExpr, Expr* aRightExpr,
>                  short aOp)

Rewrap.

>+    txNumberExpr(Expr* aLeftExpr,
>+                 Expr* aRightExpr,
>+                 eOp aOp)

Rewrap.

>+    RelationalExpr(Expr* aLeftExpr, Expr* aRightExpr,
>                    RelationalExprType aOp)

Rewrap.

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

>@@ -172,21 +146,17 @@ FunctionCall::toString(nsAString& aDest)

>+    for (PRUint32 i = 0; i < mParams.Length(); ++i) {

Nit: I prefer the declaration of i outside the for.

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

> void PredicateList::toString(nsAString& dest)

>+    for (PRUint32 i = 0; i < mPredicates.Length(); ++i) {

Nit: I prefer the declaration of i outside the for.

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

>@@ -468,16 +468,23 @@ txStylesheet::addTemplate(txTemplateItem

>+            unionPos++;

Nit: ++unionPos;

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

> txUnionPattern::toString(nsAString& aDest)

>+    for (PRUint32 i = 0; i < mLocPathPatterns.Length(); ++i) {

Nit: I prefer the declaration of i outside the for.

> txLocPathPattern::setSubPatternAt(PRUint32 aPos, txPattern* aPattern)

>+    NS_ASSERTION(aPos < mSteps.Length(),
>                  "setting bad subexpression index");

Rewrap.

> txLocPathPattern::toString(nsAString& aDest)

>+    for (PRUint32 i = 0; i < mSteps.Length(); ++i) {

Nit: I prefer the declaration of i outside the for.

>Index: src/xslt/txXSLTPatterns.h
>===================================================================

> class txPattern : public TxObject
> {
> public:
>-    virtual ~txPattern();
>+    virtual ~txPattern()
>+    {
>+    }

You can just remove this?

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 247792 [details] [diff] [review]
Use nsTArray and remove nsAutoPtr arguments.

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

>@@ -293,29 +293,32 @@ txXPathOptimizer::optimizeUnion(Expr* aI

>-                unionTest = new txUnionNodeTest;
>+                nsAutoPtr<txNodeTest> owner = unionTest = new txUnionNodeTest;

This needs to be:

                nsAutoPtr<txNodeTest> owner(unionTest = new txUnionNodeTest);

Comment 31

[Peter Van der Beken [:peterv]]

Comment on attachment 247792 [details] [diff] [review]
Use nsTArray and remove nsAutoPtr arguments.

You need to call nt.forget() after creating a LocationStep in:

txStylesheet::init
txFnStartApplyTemplates
txFnStartSort

Comment 32

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

Yeah, I noticed that yesterday. Must have been a last change after I ran buster. With that fixed it does pass buster for sure though. You want a new patch or should I check in with that fixed? I'm not currently at a computer whith a tree so I can't attach right now.

Comment 33

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

> >+    txLiteralExpr(double aDbl)
> >+        : mValue(new NumberResult(aDbl, nsnull))
> >+    {
> >+    }
> >+    txLiteralExpr(const nsAString& aStr)
> >+        : mValue(new StringResult(aStr, nsnull))
> >+    {
> >+    }
> 
> We need to handle out-of-memory for these.

Filed bug 363521

> >Index: src/xpath/txFunctionCall.cpp
> >===================================================================
> 
> >@@ -172,21 +146,17 @@ FunctionCall::toString(nsAString& aDest)
> 
> >+    for (PRUint32 i = 0; i < mParams.Length(); ++i) {
> 
> Nit: I prefer the declaration of i outside the for.

I did this for the toString methods since I doubt we'll muck with those very much and I want to keep them small and out-of-the way.

Comment 34

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

Attachment: Comments addressed

Comment 35

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

Comment on attachment 248345 [details] [diff] [review]
Comments addressed

Grr.. forgot to run cvs diff before attaching apparently. Will do that tomorrow.

Comment 36

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

Attachment: Comments addressed

Comment 37

[Peter Van der Beken [:peterv]]

Comment on attachment 248428 [details] [diff] [review]
Comments addressed

Don't forget about comment 30, you'll burn the tree otherwise.

Comment 38

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

Comment on attachment 248428 [details] [diff] [review]
Comments addressed

Checked in. This saved(!) 8k on linux. See http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1165961700.31897.gz&fulltext=1

Some of it was for the inlined functions, which I expected. But a lot of it seemed to be the non-explicit dtors which I can't really explain.

Comment 39

[Peter Van der Beken [:peterv]]

Patches got landed years ago, I'm going to close this as FIXED.