John, I didn't see your e-mail, but I did see bug 3477. I have changed some of our iterators already according to the fixes you suggested, and other iterators I have let be either because they have to be as they are, or because they require some further discussion. StepIterator - fixed ------------ This is a very frequently used iterator. I removed the unnecessary call to setStartNode() in the reset() method. Note that it is necessary to call setStartNode() on one of the iterators in the reset() method, but not both. When the source iterator is reset, the other iterator must be given a new start node to reflect the new position of the source iterator (I hope that sentence makes sense). ReverseIterator - fixed --------------- I have fixed the reverse iterator so that it is correctly reset when reset() is called. I think this bug is a very easy one to make, forgetting to reset local variables, and assuming that the super- class' reset() method handles everything. DOMImpl.StrippingIterator - OK ------------------------- This iterator returns nodes that it gets from another iterator. It does not have to set initialize its own _startNode, but rather calls setStartNode() and reset() on its source iterator. KeyIndex - OK -------- This iterator does not have a start node. It returns all nodes that exist under a specific key/id in an index. Calling reset() will set the iterator's position to the first node for the key or id, but setStartNode() has no effect at all. Here also, setStartNode doesn't initialize _startNode and there appears to be no ill effects since reset() is reimplemented. I do notice however that the constructor for this class calls source.setStartNode(int), possibly changing the state of a parameter! DOMImpl.OrderedIterator - needs complete re-design ----------------------- This iterator is very badly implemented. I don't think the call to setStartNode() in the constructor is a problem, as the ordered iterator is supposed to be used as a filter for certain types and combinations of iterators. The setStartNode() method should call setStartNode() on the source iterator and re-read the cached bit array, and the reset() method should reset the current node to the first node (bit) set in the array. The whole bit-array thing should be replaced by a simple vector. For this, setStartNode(int) calls _source.setStartNode(int). Reset() is implemented but does no work at all. MatchingIterator - needs fix ---------------- I will immediately start an investigation of this iterator. It seems like it does not do half the work it is intended to do. Its constructor takes an iterator and a node as input, checks if the soyrce iterator returns the node, but the matching iterator still returns all the nodes in the source. The matching iterator is used in the compiler.StepPattern class only. UnionIterator - needs complete re-design ------------- This iterator is very badly implemented as well and actually seems to need the call to setStartNode() of every iterator when reset() is called. So yes, there is a big performance hit. I'd like to see a complete re-design and implementation of this iterator. Morten John Howard wrote: > > Hi Morten, > > I presume my email got lost in the post-vacation deluge... Understandable! > I'm sending it again though because I'm still interested in hearing from you > on a few issues. > > thanks, > > john
