WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug