RESOLVED FIXED 12497
XPathEvaluator sometimes fails to order nodes in its result
https://bugs.webkit.org/show_bug.cgi?id=12497
Summary XPathEvaluator sometimes fails to order nodes in its result
Alexey Proskuryakov
Reported 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.
Attachments
proposed fix (74.77 KB, patch)
2007-03-17 13:11 PDT, Alexey Proskuryakov
darin: review+
Alexey Proskuryakov
Comment 1 2007-03-17 13:11:18 PDT
Created attachment 13683 [details] proposed fix
Darin Adler
Comment 2 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!
Alexey Proskuryakov
Comment 3 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).
Darin Adler
Comment 4 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.
Darin Adler
Comment 5 2007-03-19 23:31:05 PDT
Comment on attachment 13683 [details] proposed fix r=me
Alexey Proskuryakov
Comment 6 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.
Note You need to log in before you can comment on or make changes to this bug.