PLDHashTableEnumeratorImpl::IsDone will never return true

VERIFIED FIXED

Status

()

P1
normal
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: dbradley, Assigned: dbradley)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Blocks: 106672
(Assignee)

Comment 1

17 years ago
Created attachment 55059 [details] [diff] [review]
Option 2, fix Next to increment mCurrent past the end
(Assignee)

Comment 2

17 years ago
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.

Comment 3

17 years ago
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 5

17 years ago
Comment on attachment 55059 [details] [diff] [review]
Option 2, fix Next to increment mCurrent past the end

r/sr=jband
Attachment #55059 - Flags: superreview+

Comment 6

17 years ago
bug 106672 should not live even one more day. Someone please mark this reviewed 
and let dbradley check it in.
Priority: -- → P1

Comment 7

17 years ago
Comment on attachment 55059 [details] [diff] [review]
Option 2, fix Next to increment mCurrent past the end

looks fine
Attachment #55059 - Flags: review+

Comment 8

17 years ago
David, can you check this in?
Assignee: neeti → dbradley
(Assignee)

Comment 9

17 years ago
Yes no problem, I'll have it in today baring no major tree closures.
Status: NEW → ASSIGNED
(Assignee)

Comment 10

17 years ago
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

Comment 11

17 years ago
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.