nsVoidArray::ElementAt and ::SafeElementAt shouldn't check mImpl

RESOLVED FIXED in mozilla1.3beta

Status

()

Core
XPCOM
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: sicking, Assigned: jesup)

Tracking

({perf})

Trunk
mozilla1.3beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Is there a reason that nsVoidArray::ElementAt and ::SafeElementAt does

return mImpl ? mImpl->mArray[aIndex] : nsnull;

instead of

return mImpl->mArray[aIndex];

*after* checking that aIndex is within bounds. For SafeElementAt it should be
compleatly safe to do the latter.

For ElementAt there one case where we would crash where we didn't used to crash;
if an empty array is accessed with a negative index. We have however asserted on
that for quite some time.

::ElementAt and ::SafeElementAt are inlined and rather heavily used so getting
rid of the extra check should at least save us some codesize.


Doug T added those extra checks as part of the checkin for bug 115853. However
there is no mention in the bug of why.
(Assignee)

Comment 1

16 years ago
Doug: I'm going to propose undoing the changes you made to nsVoidArray.h in bug
115853.  The only case where something would happen differently is if the index
was negative, in which case it should be Asserting already.

Taking bug.  Patch to follow.
Assignee: dougt → rjesup
Keywords: perf
Target Milestone: --- → mozilla1.3beta
(Assignee)

Comment 2

16 years ago
Created attachment 111088 [details] [diff] [review]
Patch to undo changes to nsVoidArray.h

This patch undoes the changes and adds a comment.
(Assignee)

Updated

16 years ago
Attachment #111088 - Flags: review?(dougt)
(Assignee)

Comment 3

16 years ago
Though it may be a separate bug I should open, I should note that we can
probably remove the safety-valve from ElementAt() now (as per the comments in
nsVoidArray.h):

    // This will go away once all assertions are handled and we feel
    // comfortable that there aren't any more out there.  Negative values
    // are all handled I think.
    if (aIndex >= Count())
    {
      return nsnull;
    }
Status: NEW → ASSIGNED
(Assignee)

Comment 4

16 years ago
See bug 96108 for discussion of the bounds-checking removal
(Assignee)

Comment 5

16 years ago
Adding brendan since he was involved in the whole bug 96108 stuff

Comment 6

16 years ago
why do you want to roll back this change?  As I remember, mImpl can be null.  
dougt: looking at the code for ::SafeElementAt i can't understand how mImpl can
be null.

In ::ElementAt it could in theory, but by then someone would have made a bad
call and we would have asserted. If that happens then we should fix the caller,
not work around it in ::ElementAt.

Comment 8

16 years ago
i am not against rolling the change back.  In fact, I am not sure why i checked
the code in -- the bug numer I cited is for something all together different.  

if you and randell have reviewed the code and agree that mImpl can not be null,
then feel free to check in the patch.

Comment 9

15 years ago
Comment on attachment 111088 [details] [diff] [review]
Patch to undo changes to nsVoidArray.h

sure.  also see 196217. if you want to land both, feel free. (probaby want to
do it before beta.)
Attachment #111088 - Flags: review?(dougt) → review+
This has been fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.