On Mon, Dec 10, 2007 at 05:44:08PM +0100, Johannes Hofmann wrote:
Just one thing. I would rather have tested the return value of getTopIterator() for NULL.
Heh, that *was* my original implementation of isEmpty() . Then I noticed the hasContents member so I used that since it shortened the code. I believe hasContents could be eliminated since it is true when and only when the stack is non-empty, though I haven't exhaustively checked this.
I think the code is generally a bit optimistic here by assuming non-NULL return value from getTopIterator(). Even if this is now always the case with your fix, it may change again in the future.
Yes it may. But I think much of the code assumes this, so if this changes in the future then a lot of code will need to be reviewed, preferably by someone who understands the design.
What do you think, if we add checks for NULL at those three points in the code where getTopIterator() is called, do we still need to check with isEmpty() to call resetLink() / resetSelection() in buttonPress()?
Yes, otherwise you get a crash in DeepIterator::compareTo, which also assumes that the stack is non-empty. (In fact the comments suggest that it assumes the tops of the stacks of the two DeepIterator objects point to the same widget.) When I found that crash I decided to give up on rewriting lots of code to handle empty DeepIterators and instead simply prevent empty DeepIterators appearing in Selection objects. Even if other code ought to handle empty DeepIterators better, I think it's reasonable to argue that Selection objects should never use them. After all, what could it possible mean for a selection to begin or end at an empty DeepIterator? So I think my bugfix makes Selections do the right thing, even if other code should be checking getTopIterator() as you suggest. Regards, Jeremy Henty