IsDone is based upon mCurrent being equal mCount. This will never occur, because Next prechecks the value of mCurrent before incrementing. So either: 1. IsDone's check is changed to mCurrent == mCount -1 Or 2. Next is changed to increment and then test I think 1 would probably have less side effects. But I think what needs to be answered is what does it mean to be Done? For instance if option 1 is chosen then we could be done but still be able to call CurrentItem. This would not be true of option 2.
Created attachment 55059 [details] [diff] [review] Option 2, fix Next to increment mCurrent past the end
I think this makes things more consistent with respect an end condition. After further thought I think 2 is the more correct solution. Not sure if it will have any negative impact its users.
brendan, do you have cycles to look at this?
Assignee: dougt → brendan
I didn't write this baby (I did review it though :-(), and I don't have cycles right now (JS double hashing and fastload bugs take precedence). I think neeti should look at this, and I'm cc'ing shaver for advice too. /be
Assignee: brendan → neeti
Comment on attachment 55059 [details] [diff] [review] Option 2, fix Next to increment mCurrent past the end r/sr=jband
Attachment #55059 - Flags: superreview+
bug 106672 should not live even one more day. Someone please mark this reviewed and let dbradley check it in.
Priority: -- → P1
Comment on attachment 55059 [details] [diff] [review] Option 2, fix Next to increment mCurrent past the end looks fine
Attachment #55059 - Flags: review+
David, can you check this in?
Assignee: neeti → dbradley
Yes no problem, I'll have it in today baring no major tree closures.
Status: NEW → ASSIGNED
Patch checked in FYI: I also noticed that mCurrent == mCount for end is used in other iterators, so this is in line with existing code.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.