114166 - rewrite nsDeque

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Akkana Peck]

nsDeque has various repeated comments, comments that refer to functions other
than the function they preceed, etc.  Timeless has volunteered to clean it up. 
I can review.

Comment 1

[timeless]

Attachment: review only combined code+comment rewrite

Comment 2

[timeless]

Attachment: -u, what i would commit if i were doing this in one pass

there are 2 silly whitespaces in the license block!!

I think i still plan to commit the code rewrite as a separate cvs pass.

Comment 3

[timeless]

Attachment: diff against 61239

1. removing all @updates. cvs log/blame handle this
2. % isn't guaranteed.	See long comment in patch
3. ::PopFront diffed really poorly. here's the code:

void* nsDeque::PopFront() {
  void* result=0;
  if (mSize>0) {
    NS_ASSERTION(mOrigin < mCapacity, "Error: Bad origin");
    result=mData[mOrigin];
    mData[mOrigin++]=0;     //zero it out for debugging purposes.
    mSize--;
    // Cycle around if we pop off the end
    // and reset origin if when we pop the last element
    if (mCapacity==mOrigin || !mSize) {
      mOrigin=0;
    }
  }
  return result;
}

Comment 4

[timeless]

Attachment: mozilla/xpcom/tests/TestDeque.cpp

Comment 5

[timeless]

Attachment: current patch from my tree

I've been running with this patch for two months i guess.

Comment 6

[timeless]

Attachment: fix callers

some callers do bad things, peeking empty deques, iterating past the end of
their deque.

Comment 7

[timeless]

Attachment: really fix callers

Comment 8

[Alec Flett]

Comment on attachment 70194 [details]
mozilla/xpcom/tests/TestDeque.cpp

rs=alecf

Comment 9

[Alec Flett]

Comment on attachment 70195 [details] [diff] [review]
current patch from my tree

I guess this looks ok - much of it is just reformatting existing code, but I
think I understand the bounds issues as well. sr=alecf
(I'm assuming akkana will also give this a good r=)

Comment 10

[Akkana Peck]

r=akkana on 70195 ("current patch from my tree") and 70205 ("really fix
callers"), with a few changes that timeless and I discussed (add a few more
clarifying comments, fix three lines in nsFind.cpp, remove a line from the test
makefile).

In addition to cleaning up and documenting the code, this also fixes a crash in
the new editor find and replace dialog (it was disabled because it was crashing
in nsDeque code) and so will unblock bug 80805.

Comment 11

[timeless]

Attachment: much better comments and a new GrowCapacity()

i'm changing the signature of GrowCapacity, it's only used internally and it
means that I can almost return an error if GrowCapacity can't grow.

Comment 12

[timeless]

Attachment: fix callers

the new find incorrectly used an iterator, fix is here and the much better
comments explains what was being done wrong.

Comment 13

[Akkana Peck]

r=akkana on the new changes -- the improved comments, the new GrowCapacity
signature, and the nsFind fix.

Comment 14

[Simon Fraser [no longer active]]

Note that this checkin caused the regression described in bug 126797, which kin
has fixed.

Comment 15

[timeless]

Attachment: last bits of cleanup (typos, one inconsistency in javadoc)

Comment 16

[Darin Fisher]

Comment on attachment 72500 [details] [diff] [review]
last bits of cleanup (typos, one inconsistency in javadoc)

sr=darin

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 72500 [details] [diff] [review]
last bits of cleanup (typos, one inconsistency in javadoc)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 18

[timeless]

fixed checked into trunk