Closed Bug 716716 Opened 8 years ago Closed 7 years ago

[CC] Add iterator over real nsPurpleBuffer entries

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The purple buffer is a weird linked list of blocks where some elements are used and some aren't, depending whether a flag is set.  Iterating over the actual live elements in a purple buffer is pretty gross, and is done in nsPurpleBuffer::NoteAll, nsPurpleBuffer::SelectPointers and nsPurpleBuffer::UnmarkRemainingPurple, and another place will be added in bug 716518.  We should have some kind of real iterator.  I don't know if the way to go is to have some kind of iterator class like with NodePool and EdgePool, or we could use some kind of template magic.  The inner loops just take a pointer to a purple buffer entry.
Luke, how awful is the design of this iterator?  It is an iterator over an array, where we skip entries that don't have their low bit tagged.  I tried overloading the -> operator for it so we could just grab the mObject out of it without the fairly ugly GetNext, but then I wasn't sure how to go from an iterator to the underlying object, to pass it to the function Remove.
Assignee: nobody → continuation
Attachment #590364 - Flags: feedback?(luke)
Comment on attachment 590364 [details] [diff] [review]
add iterator over live purple buffer entries in a block

You could consider overloading both operator->() and operator*() where operator* returns an nsPurpleBufferEntry&.

Also: can aBlock->mEntries->mObject have the bit set?  If so, the constructor will need to do the same skip-until-not-set as operator++.
Attachment #590364 - Flags: feedback?(luke) → feedback+
(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 590364 [details] [diff] [review]
> add iterator over live purple buffer entries in a block
> 
> You could consider overloading both operator->() and operator*() where
> operator* returns an nsPurpleBufferEntry&.

Yeah, I tried that, but doing Remove(i) still gave an error.  Maybe I'll try again in case I was doing something slightly wrong.

> Also: can aBlock->mEntries->mObject have the bit set?  If so, the
> constructor will need to do the same skip-until-not-set as operator++.

D'oh, I guess that's why it is crashing immediately...

Thanks!
Comment on attachment 590364 [details] [diff] [review]
add iterator over live purple buffer entries in a block

I messed around with a templatey implementation of iterating over purple buffer entries and it looks a lot nicer to me than this.
Attachment #590364 - Attachment is obsolete: true
OO closures unfortunately make this a little ugly.
I have this blocking ICC, but I suppose I could just mash another function into the purple buffer that does yet another copy paste of the iteration code, so it isn't really blocking.
Attachment #725819 - Attachment is obsolete: true
bonus code:
- removed the unused StaticAsserts()
- made the fields of nsPurpleBuffer private
- Fixed the adjustment of mCount in UnmarkRemainingPurple: before we were doing this for every entry, whether or not there was anything in it!  Totally bogus.  We should consider just crashing the browser if mCount != 0 which would let us tear out UnmarkRemainingPurple, which likely is never run ever, but I'm afraid of how that would go.

Otherwise the behavior should not be changed.
Attachment #742536 - Flags: review?(bugs) → review+
Duplicate of this bug: 865731
https://hg.mozilla.org/mozilla-central/rev/d696aa2a413d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 867974
You need to log in before you can comment on or make changes to this bug.