95059 - Fix *Expr.cpp

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Axel Hecht]

I looked over *Expr.cpp and found a few bugs:

FilterExpr::matches must evaluate with context node, not node.
TextExpr needs CDATA
RootExpr::matches is wrong for multiple nodes.
ElementExpr and AttributeExpr should get NameTestExpr, there is something
bad with wildcards in there.

BooleanExpr.cpp could use some newlines in if ()

Each file should have a comment with the XPath expression it implements

Comment 1

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

What is the context node when calling ::matches? A pattern matches if the 
tested node is part of the resulting nodeset when the expression is evaluated 
in *some* context, not any specific context.
The only filter-expressions that are valid patterns are "id('Literal')" and "key
('Literal', 'Literal')" and neither of them are context sensitive. (Well, they 
are sensitive as to what document the context node lives in, and they only 
return nodes within that document.) I could imagine that there are some bugs in 
the interaction between PathExpr and FilterExpr wrt matching, but I would think 
that there is a bug in PathExpr in that case (though I could be wrong of corse).
IMHO the context argument should be removed from ::matches
(and ::getDefaultPriority) and I'll do that as soon as bug 84677 is checked in 
to avoid too much merge hell.

If I'm totally wrong, could you provide an example pattern of when we need the 
context argument.

I don't really understand what you mean about multiple nodes in 
RootExpr::matches since matching is done on one node at a time.

IMHO we shouldn't spend too much time on ElementExpr and AttributeExpr since we 
should merge them into a NameTest class (which dosn't inherit Expr at all) as 
soon as bug 76070 is fixed.

Comment 2

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

not sure what way to do the dependency on bug 92786

Comment 3

[Axel Hecht]

::matches is always with a context node. It's different to xpath's definition
of matches. And FilterExpr uses the node to be tested as context, not the 
context node.

And RootExpr::matches is wrong for multiple documents. Sorry, didn't say that
precisely enough.

Dep is the other way around, IMHO.

Axel

Comment 4

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

I still think we should scrap the |Node* context| argument. Currently all 
implementations of ::matches returns true if the "node" argument matches in 
*some* (valid if you will) context.

IMHO it should be the job of ::matches to test agains every valid context if it 
needs to. So what could be correct is if you in FilterExpr::matches walk up 
from the tree from the "node" argument and evaluate until you find get a 
resulting nodeset which contains the tested node.
Don't worry about if it's slow since no valid patterns should produce a 
FilterExpr ;)

Why is RootExpr::matches wrong wrt multiple documents? IMHO "/" should match 
the root of any document.

Comment 5

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

I should add an "I think" between "FilterExpr" and ";)"

Comment 6

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

You proboboly wanna do the same with FunctionCall::matches too. I'm still not 
sure if it's the RightThing yet though...

Comment 7

[Axel Hecht]

Ok. I have digged a bit further.
::matches is only used for XSLT Patterns and whitespace stripping.
so...
FilterExpr::matches should throw an assertion. IMHO.
FunctionCall::matchse should throw an assertion if it's not key() or id().
context can go.
RootExpr doesn't matter, as PathExpr will always stay within the same doc.

Still contras?

Axel

Comment 8

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

/me shakes hands with Pike

Agreed :)

Comment 9

[Axel Hecht]

context must die in getDefaultPriority, too.

I wonder if we should remove the assertion from Expr::getDefaultPriority and
use it for all objects that return 0.5

Axel

Comment 10

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

IMHO no, since that's a good catch of if we've forgotten to 
implement ::getDefaultPriority somewhere.

Comment 11

[Peter Van der Beken [:peterv]]

Suckage. I just witnessed that @ped:* doesn't match ped:test="foo".
AttributeExpr should bail out early if namespaceID == kNameSpaceID_XMLNS.

Comment 12

[Peter Van der Beken [:peterv]]

How much of this was fixed by contexts landing?

Comment 13

[Axel Hecht]

all.

Comment 14

[Axel Hecht]

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