Closed Bug 807713 Opened 8 years ago Closed 8 years ago

Add comment warning about nsTObserverArray ElementAt and why there is no operator[]

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(2 files, 1 obsolete file)

I noticed this while poking around in bug 806279. The other major array types seem to, and I can't think offhand of a reason not to have one here, too.
I just cribbed this code from nsTArray. I also converted one use of it to make sure it is okay, but I can remove that if you want.
Assignee: nobody → continuation
Attachment #677476 - Flags: review?(benjamin)
Attachment #677476 - Flags: review?(benjamin) → review+
Thanks for the review!

The world's slowest Linux32 try run I did just to be careful finally came back green as expected. Hopefully this doesn't break MSVC somehow.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3e819fda43
https://hg.mozilla.org/mozilla-central/rev/5c3e819fda43

Should this have a test?
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Nah, my patch uses one of the new functions in nsAccessiblePivot, and this is just very simple code copied from nsTArray so it is probably okay.
Flags: in-testsuite? → in-testsuite-
(In reply to Andrew McCreight [:mccr8] from comment #0)
> The other major array types seem to, and I can't think offhand of a reason not to
> have one here, too.

There is debate going all the way back to bug 342252 comment 2.  It's a footgun and you probably want to use an iterator.
Depends on: 342252
Yeah, this should be backed out. This array exists explicitly so that we can deal with the array changing under us, and these index operators doesn't do that.
Okay, I can put together a patch that backs this out, and adds a little comment explaining why there is no operator[]. I guess I should also add a comment on ElementAt that says not to avoid using it? Is SafeElementAt okay to use, or should it be avoided, too?
Sounds great. SafeElementAt should be fine.

Thanks!
Attachment #684737 - Flags: review?(jonas)
Missed a case.  I guess this isn't actually used.
Attachment #684737 - Attachment is obsolete: true
Attachment #684737 - Flags: review?(jonas)
Attachment #684744 - Flags: review?(jonas)
Comment on attachment 684744 [details] [diff] [review]
backout, add comment explaining why

Review of attachment 684744 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!
Attachment #684744 - Flags: review?(jonas) → review+
well, backed out from 20. I'll put it up for Aurora approval later.
Comment on attachment 684744 [details] [diff] [review]
backout, add comment explaining why

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this is a backout patch, plus it adds some comments
User impact if declined: this patch added some new C++ functions to 19 that are a bad idea, so I just want to remove it from there so 19 is consistent with 18 and 20.
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): really low. the new functions are just wrappers, and are almost unused, and this just restores the status quo anyways
String or UUID changes made by this patch: none
Attachment #684744 - Flags: approval-mozilla-aurora?
Attachment #684744 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/17fa80b258dd

Okay, I just morphed this bug to be about adding a comment, not about adding operator[].
Summary: nsTObserverArray does not define an operator[] → Add comment warning about nsTObserverArray ElementAt and why there is no operator[]
Can someone please attach a testcase ?
This doesn't have a test case, it just adds a comment.
You need to log in before you can comment on or make changes to this bug.