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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: add and use nsTObserverArray::operator[]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c3e819fda43

Should this have a test?

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Jesse Ruderman]

(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.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Andrew McCreight [:mccr8]]

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?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds great. SafeElementAt should be fine.

Thanks!

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: backout, add comment explaining why

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: backout, add comment explaining why

Missed a case.  I guess this isn't actually used.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 684744 [details] [diff] [review]
backout, add comment explaining why

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

Thanks for doing this!

Comment 12

[Andrew McCreight [:mccr8]]

landed in 20
https://hg.mozilla.org/integration/mozilla-inbound/rev/8849063cf059

Comment 13

[Andrew McCreight [:mccr8]]

well, backed out from 20. I'll put it up for Aurora approval later.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8849063cf059

Comment 15

[Andrew McCreight [:mccr8]]

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

Comment 16

[Andrew McCreight [:mccr8]]

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[].

Comment 17

[Mihai Morar, (:MihaiMorar)]

Can someone please attach a testcase ?

Comment 18

[Andrew McCreight [:mccr8]]

This doesn't have a test case, it just adds a comment.