Closed Bug 807713 Opened 8 years ago Closed 8 years ago
Add comment warning about ns
TObserver Array Element At and why there is no operator
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)
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
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.
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!
Missed a case. I guess this isn't actually used.
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.