102293 - We should have proper NodeTests

Status: VERIFIED FIXED

(Core :: XSLT, defect, P2)

Description

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

We should have proper NodeTests instead of letting everything being 
expressions. The classes AttributeExpr, BasicNodeExpr, ElementExpr, and 
TextExpr are currently only used as NodeTests (ie we never call ::evaluate on 
them) even though they inherit Expr.

The reason we need NodeTests instead of just expressions is that there is a 
difference between the the expression "node()" and the nodetest "node()":

* The expression "node()" is really short for "child::node()" which only
  matches nodes that has a parent, ei it does not match root-node.
* The nodetest "node()" matches any node, so for example when used in the
  ancestor axis in the expression "ancestor::node()" the root-node will be part
  of the resulting nodeset.

So what I've done is to create a new rootclass, txNodeTest. I've then merged 
ElementExpr and AttributeExpr into txNameTest. BasicNodeExpr and TextExpr is 
merged into txNodeTypeTest (should it be called txNodeType?).

Warning! the names in the enum for wildcards sux! suggestions appritiated!

Comment 1

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

Attachment: here goes... diff -N

Comment 2

[Axel Hecht]

txNodeTest::matches shouldn't have a context state. ErrorObserver should be
enough. (And that indicates more clearly what's supposed to happen.)

+    /*
+     * Creates a new txNameTest with the given type and the given
+     * principal node type
+     */
+    txNameTest(String& aName, Node::NodeType aNodeType);
TypeType?
And make that Node::NodeType an enum over element or attribute.
txNameTest should resolve the namespace in the constructor. Give it a 
ContextState, I gave you the dependency for that.
I like WildNess, btw.
txNameTest::matches should be stricter to attributes or elements. The parent of
a attr is always an element, no need to check that. Of course you could null 
check it.
So far for this bug and today.

Axel

Comment 3

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

> txNodeTest::matches shouldn't have a context state. ErrorObserver should be
> enough. (And that indicates more clearly what's supposed to happen.)

No, i want it to have a complete contextstate so that we can make some more 
powerfull in the future. For example in an expression like "foo[@bar]" it would 
be great if have a special NodeTest that combined the predicate and the 
NameTest (this is part of the big optimisations thingy that we'll do in 
2010 :) )

> +    txNameTest(String& aName, Node::NodeType aNodeType);
> TypeType?
> And make that Node::NodeType an enum over element or attribute.

Hmm.. I rather like it to have it generic for any nodetype. This way we can 
also use this namespace nodes.

> txNameTest should resolve the namespace in the constructor. Give it a 
> ContextState, I gave you the dependency for that.

I totally agree that this is the right way to do it. Not really sure I wanna 
wait that long though, but I could live with it I guess.

> I like WildNess, btw.

If you're happy, I'm happy :)

> txNameTest::matches should be stricter to attributes or elements. The parent
> of a attr is always an element, no need to check that. Of course you could
> null check it.

Acually this won't be a problem once ns-doom has landed and bug 96410 is fixed. 
The code will look totally different then.

Comment 4

[Peter Van der Beken [:peterv]]

So nsdoom has landed. Show us the difference ;).

Comment 5

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

hrm, actually I need bug 96410 too

Comment 6

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

Attachment: ver 2

Comment 7

[Axel Hecht]

push. This bug depends on the xpath context foo, which didn't make it.
At least we can't decide if it does or not fast enough.
Sorry Jonas, I owe you a trout.

Comment 8

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

pushing

Comment 9

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

this will be part of Pikes context rework

Comment 10

[Axel Hecht]

these bugs should go in with bug 113611

Comment 11

[Axel Hecht]

fixed with checkin of bug 113611.

Comment 12

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.