70861 - context nodeset is wrong in LocationStep::matches

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

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

LocationStep does the wrong thing in ::matches. It adds only the context node 
to the context nodeset when evaluating the predicates. I'll add a patch that 
does the right thing, which is very slow :(

Comment 1

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

Attachment: patch to fix (linenumbers might be wrong but should work anyway)

Comment 2

[Axel Hecht]

that patch doesn't work. I'm afraid we need to get the complete evaluate of 
the location step, and use that nodeset for the predicates.

letter[position()=3] is not working with that patch. only last() does.

Watch out for stuff like 

<?xml version="1.0"?> 
<letters>
  <letter>a</letter>
  <letter>b</letter>
  <looser>failed</looser>
  <letter>c</letter>
  <letter>b</letter>
  <letter>h</letter>
</letters>

I didn't understand the programflow in that part completely, Keith, could you
give a hint on this? Moving to you, I don't grok this good enough

Axel.

Comment 3

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

Attachment: evaluate predicates only once

Comment 4

[Axel Hecht]

This patch works.
I'm not sure though if we wouldn't be better off alot if we evaluated the 
Pattern and cached matching nodes (with context node), and just check for 
membership. This might be a good idea at least for templates with a decent
priority. (priority as measure of expected match work vs. memory demand came
just to my mind).
Should we get this in, and keep a better match procedure in mind? I'd comment
in the "faster!" bug then.

Axel

Comment 5

[Axel Hecht]

what should bla/fasel/junk[position()=last()] be?
IIRC, this patch sets the context node to the parent, but in an expression like
the above, the context node should be the parent of the bla.

Right?

Axel

Comment 6

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

nope, bla/fasel/junk[position()=last()] means "in *every* bla/fasel select the 
last <junk>". Ie not "the last bla/fasel/junk", that can be done with
(bla/fales/junk)[position()=last()].

So what the patch does is
1. check that the node to mach is an element named "junk"
2. step up to the <junk>s parent
3. evaluate junk[position()=last()] (which returns the last <junk> sibling)
4. check if the mached node is contained in the returned nodelist
5. if so, return true

control then returns to ExprParser which checks that the parent of the node is 
a <fasel>, and it's parent is a <bla>.

Comment 7

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

the fix for this is included in last patch in bug 70865

Comment 8

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

fix in bug 70865 checked in

Comment 9

[Axel Hecht]

bitching buttons, verfication spam