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.
Created attachment 13683 [details] proposed fix
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!
(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).
(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 on attachment 13683 [details] proposed fix r=me
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.