Bug 12497 - XPathEvaluator sometimes fails to order nodes in its result
Summary: XPathEvaluator sometimes fails to order nodes in its result
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-31 02:26 PST by Alexey Proskuryakov
Modified: 2007-03-20 10:40 PDT (History)
0 users

See Also:


Attachments
proposed fix (74.77 KB, patch)
2007-03-17 13:11 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-01-31 02:26:31 PST
At least calculating id() and union ("|") has this problem.

Currently, the assumption is that all functions should preserve document order on nodes, so I added FIXME comments to these functions. However, it may be better to just sort the nodes after the fact, and only if the client has requested ORDERED_NODE_SNAPSHOT_TYPE, ORDERED_NODE_ITERATOR_TYPE or FIRST_ORDERED_NODE_TYPE result.
Comment 1 Alexey Proskuryakov 2007-03-17 13:11:18 PDT
Created attachment 13683 [details]
proposed fix
Comment 2 Darin Adler 2007-03-19 20:34:37 PDT
Comment on attachment 13683 [details]
proposed fix

+    const_cast<Vector<RefPtr<Node> >& >(m_nodes).swap(sortedNodes);

+        const_cast<bool&>(m_isSorted) = true;

Should use mutable instead of const_cast in these places.

NodeSet::sort does not seem to set m_isSorted in the normal case.

Since NodeSet is used as a return value in various places, it needs to be implemented in a way that's efficient to copy. Having a member that's a Vector makes it quite inefficient to copy!
Comment 3 Alexey Proskuryakov 2007-03-19 22:41:20 PDT
(In reply to comment #2)

> Should use mutable instead of const_cast in these places.

  I have considered that, but mutable affects all methods, while there's only one that wants to change these values. So, ieemed safer to  use const_cast.

> NodeSet::sort does not seem to set m_isSorted in the normal case.

  It does implicitly (new NodeSets are created with m_isSorted == true, so it's swapped with a sorted one).

> Since NodeSet is used as a return value in various places, it needs to be
> implemented in a way that's efficient to copy. Having a member that's a Vector
> makes it quite inefficient to copy!

  I'm planning to investigate this later (as you suggested before, it might be better to stop returning it by value).
Comment 4 Darin Adler 2007-03-19 23:30:32 PDT
(In reply to comment #3)
> > Should use mutable instead of const_cast in these places.
> 
>   I have considered that, but mutable affects all methods, while there's only
> one that wants to change these values. So, ieemed safer to  use const_cast.

It might be more localized and hence safer, but I believe it's not standard-compliant. You can't cast away const unless you know you started with a non-const at some point. I believe the use of const_cast here is simply incorrect.
Comment 5 Darin Adler 2007-03-19 23:31:05 PDT
Comment on attachment 13683 [details]
proposed fix

r=me
Comment 6 Alexey Proskuryakov 2007-03-20 10:40:10 PDT
Committed revision 20345.

(In reply to comment #4)
> It might be more localized and hence safer, but I believe it's not
> standard-compliant. You can't cast away const unless you know you started with
> a non-const at some point.

Currently, all NodeSets are created non-const indeed, and it seems that even if this accidentally changes, negative consequences are fairly unlikely, so landed as is.