820403 - replace some nsISupportsArray members with nsCOMArrays

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsISupportsArray members in nsFileView.cpp

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsISupportsArray members in nsFormFillController

Comment 3

[(no longer active)]

Comment on attachment 690899 [details] [diff] [review]
remove nsISupportsArray members in nsFormFillController

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

Actually, you should probably convert these to nsTArray<nsCOMPtr<nsIFoo> > so that the arrays have the same interface as nsCOMArray.  We should not add more nsCOMArray's to the code base, and I was stupid for not thinking of this sooner.  :(

Comment 4

[Benjamin Smedberg]

Ehsan really? I thought that we still preferred nsCOMArray to nsTArray<nsCOMPtr>.

Comment 5

[(no longer active)]

(In reply to comment #4)
> Ehsan really? I thought that we still preferred nsCOMArray to
> nsTArray<nsCOMPtr>.

Do we?  I thought we hated nsCOMArray because its interface is different to nsTArray which is what people are used to!  :-)

Hrm, let me put it this way, if I came up with a patch which converted all nsCOMArray's to nsTArray<nsCOMPtr>'s, and typedef'ed nsCOMArray<T> to nsTArray<nsCOMPtr<T> > (let's pretend that C++ lets us define such a typedef ;-), would you take that patch?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, the different interface is awful. We should just have one array class and typedef if people like the way that 'nsCOMArray' sounds.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Hrm, let me put it this way, if I came up with a patch which converted all
> nsCOMArray's to nsTArray<nsCOMPtr>'s, and typedef'ed nsCOMArray<T> to
> nsTArray<nsCOMPtr<T> > (let's pretend that C++ lets us define such a typedef
> ;-), would you take that patch?

I actually started looking at that, but there was too much to do, so I focused on the smaller task of eliminating voidArray, in bug 819090.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 690897 [details] [diff] [review]
remove nsISupportsArray members in nsFileView.cpp

FWIW, this patch is more-or-less what was done in bug 792566; that patch used nsTArray.

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: bug 820403 - remove nsISupportsArray members in nsFormFillController

now using TArray

Comment 10

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9929a498ba0e

Comment 11

[[e-mail]]

(In reply to Nathan Froyd from comment #8)
> Comment on attachment 690897 [details] [diff] [review]
> remove nsISupportsArray members in nsFileView.cpp
> 
> FWIW, this patch is more-or-less what was done in bug 792566; that patch
> used nsTArray.

No, this patch is way better than the one in bug 792566 :-P

Comment 12

[[e-mail]]

(In reply to Ehsan Akhgari from comment #5)
> Hrm, let me put it this way, if I came up with a patch which converted all
> nsCOMArray's to nsTArray<nsCOMPtr>'s, and typedef'ed nsCOMArray<T> to
> nsTArray<nsCOMPtr<T> > (let's pretend that C++ lets us define such a typedef
> ;-), would you take that patch?

Although it does at least now support SafeElementAt, nsTArray still doesn't have all the APIs that nsCOMArray does. (In fact, nsISupportsArray has an API that no other array does, forcing Standard8 to emulate it in bug 820377...)

Comment 13

[Benjamin Smedberg]

AIUI, the big problem with nsTArray<nsCOMPtr> was that it hurt codesize in a rather significant way. The underlying comarray-base which saves everything as an array of nsISupports means that the different template versions of nsCOMArray are basically free.

It also means that things like bug 820674 can operate on the generic base rather than on the specialized class, so you don't have extra codesize penalties for those.

This isn't so much of an issue with the MSVC because of COMDAT folding, but none of the GCC/clang toolchains do anything equivalent, as far as I know.

If you just made the nsCOMArray API match nsTArray, that would be excellent, but still a lot of work.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> This isn't so much of an issue with the MSVC because of COMDAT folding, but
> none of the GCC/clang toolchains do anything equivalent, as far as I know.

FWIW, gold on Linux will do this if you ask it to.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9929a498ba0e

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> > This isn't so much of an issue with the MSVC because of COMDAT folding, but
> > none of the GCC/clang toolchains do anything equivalent, as far as I know.
> 
> FWIW, gold on Linux will do this if you ask it to.

Do we?  :-)

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> (In reply to comment #14)
> > (In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> > > This isn't so much of an issue with the MSVC because of COMDAT folding, but
> > > none of the GCC/clang toolchains do anything equivalent, as far as I know.
> > 
> > FWIW, gold on Linux will do this if you ask it to.
> 
> Do we?  :-)

It looks like we do: http://mxr.mozilla.org/mozilla-central/source/build/autoconf/compiler-opts.m4#111