<WeirdAl> I have an interface method written like this: |void handleArray(in PRUint32 count, [array, size_is(count)] nsIFoo fooObjects);| and I have a nsCOMArray in the caller. I'm wondering if I should change the interface to take a nsIArray, or if I should come up with a generic conversion from nsCOMArray<nsIFoo> to nsIFoo**, or if I should do something else <NeilAway> it should be possible to write a new T** nsCOMArray<T>::Elements() method that returns a flat array that you can pass as an IDL parameter <WeirdAl> your thinking parallels mine (new method) - two questions. (1) Cleaning up the memory afterwards. (2) Is there enough demand to justify adding it to nsCOMArray? I see no use cases in current C++ code. <NeilAway> there's no memory to clean up in the Elements() case, you'd just write nsCOMArray<T> tArray; /* ... */ doFoo(T.Elements(), T.Count()); <WeirdAl> I don't quite follow. How would we create the T** object in the first place? <WeirdAl> (Mozilla's C++ templates have spoiled me; it's very hard for me to write raw C++) <NeilAway> it should be possible to access the raw array, <NeilAway> possibly via a helper method on nsVoidArray <NeilAway> template<class T> class nsCOMArray : public nsCOMArray_base { ... T** Elements() { return reinterpret_cast<T**>(mArray.Elements()); } ... } <NeilAway> class nsVoidArray { ... void **Elements() { return mImpl->mArray; } ... } <NeilAway> hmm, actually mImpl could be null, I'd have to write { return mImpl ? mImpl->mArray : nsnull; } <WeirdAl> would this implementation also be useful in the outparam case? <NeilAway> no, a helper class would work best for that * WeirdAl wouldn't think so - you'd need a constructor <NeilAway> no actually, outparam is hard, because you have the data and the length <WeirdAl> so it's a two-argument helper <NeilAway> no, I mean you'd have to write another method on nsCOMArray to "adopt" an outparam <WeirdAl> ok, you and I are actually thinking on the same track <WeirdAl> now, here's the kicker: is there enough value in these ideas to warrant adding them to mozilla-central? I saw practically no demand for in params, and I haven't looked at out params <NeilAway> Adopt(T** data, PRInt32 count) { Clear(); SetSize(count); memcpy(Elements(), data, count * sizeof(T*)); NS_Free(data); } perhaps <WeirdAl> I haven't filed a bug, because I'm not sure it's needed <NeilAway> I'd start by looking though all the interfaces to see how many of them pass arrays of pointers <WeirdAl> I did that for in params - all callers were JS <WeirdAl> mozilla1.8, I did find one C++-based caller <NeilAway> numbers aren't on your side then :-( <WeirdAl> I do see six out nsI* methods in IDL that look promising, though, including nsIXTFElement... XForms guys would love that converter if they don't already have it <NeilAway> maybe you can get them to push it Code like this would be of use at Skyfire, since I have a XPIDL interface I've written to take a XPIDL array of objects - and both C++ and JS callers. mozilla.org code would find it less useful, based on a quick search: http://mxr.mozilla.org/mozilla-central/search?string=size_is&find=\.idl%24&findi=\.idl%24&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central With the help Neil provided above, I can write a patch if desired.

Any helpers we write may need an extra argument (PRBool AddRefElements or PRBool ReleaseElements). Reason: nsCOMArray::~nsCOMArray releases references, so when the array goes out of scope, we risk destroying the array elements we're trying to send out.

Strictly speaking, the move constructor/assignment operators don't need to be part of this patch, but they were useful for other work I was doing in comm-central, so I threw them in here as well.

Comment on attachment 8362735 [details] [diff] [review] Add rvalue methods, Adopt, and Forget to nsCOMArray Review of attachment 8362735 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the miscellaneous fixes below. ::: xpcom/glue/nsCOMArray.cpp @@ +279,5 @@ > > return n; > } > + > +void Nit: whitespace above "void" here. @@ +284,5 @@ > +nsCOMArray_base::Adopt(nsISupports** aElements, uint32_t aSize) > +{ > + Clear(); > + // Don't use nsCOMArray::AppendElements at here--we're stealing the > + // reference counts from the array. This seems like an odd comment, given that we're in nsCOMArray_base at this point. Delete it please. @@ +297,5 @@ > +{ > + uint32_t length = Length(); > + nsISupports** array = static_cast<nsISupports**>( > + moz_xmalloc(sizeof(nsISupports*) * length)); > + memmove(array, Elements(), sizeof(nsISupports*) * length); You could pull out the sizeof()*length into a local for these two uses; you shouldn't have to wrap the |array| declaration then. Same # of lines, but I think it reads a little clearer. ::: xpcom/glue/nsCOMArray.h @@ +404,5 @@ > + * nsCOMArray<nsISomeInterface> array; > + * nsISomeInterface** elements; > + * uint32_t length; > + * ptr->GetSomeArray(&elements, &length); > + * array.Adopt(elements, length); I think it would be good to note here that |elements| is freed. @@ +412,5 @@ > + aSize); > + } > + > + /** > + * Export the contents of this array to an XPIDL outparam. Please add a note that the array is Clear()'d after this operation.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e057ecce0007 because of Windows build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=33363925&tree=Mozilla-Inbound Processing config: Processing plugin dlls: "c:\mozilla-build\nsis-2.46u\Plugins\*.dll" webapprt.cpp webapprt-stub.exe c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\config\rules.mk:744:0$ c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/expandlibs_exec.py --depend .deps/webapprt-stub.exe.pp --target webapprt-stub.exe --uselist -- link -NOLOGO -OUT:webapprt-stub.exe -PDB:webapprt-stub.pdb -ENTRY:wmainCRTStartup -SUBSYSTEM:WINDOWS -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF /HEAP:0x40000 webapprt.obj ./module.res c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/dist/lib/xpcomglue_staticruntime.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib shell32.lib c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\config\rules.mk:744:0: command 'c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/expandlibs_exec.py --depend .deps/webapprt-stub.exe.pp --target webapprt-stub.exe --uselist -- link -NOLOGO -OUT:webapprt-stub.exe -PDB:webapprt-stub.pdb -ENTRY:wmainCRTStartup -SUBSYSTEM:WINDOWS -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF /HEAP:0x40000 webapprt.obj ./module.res c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/dist/lib/xpcomglue_staticruntime.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib shell32.lib' failed, return code 1120 <libs>: Found error <../../dist/bin/webapprt-stub.exe>: Found error <../../dist/bin/webapprt-stub.exe>: Found error <libs>: Found error Executing: link -NOLOGO -OUT:webapprt-stub.exe -PDB:webapprt-stub.pdb -ENTRY:wmainCRTStartup -SUBSYSTEM:WINDOWS -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF /HEAP:0x40000 @c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\webapprt\win\tmpkoxtjh.list module.res ..\..\dist\lib\xpcomglue_staticruntime.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib shell32.lib c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\webapprt\win\tmpkoxtjh.list: webapprt.obj xpcomglue_staticruntime.lib(nsCOMArray.obj) : error LNK2019: unresolved external symbol __imp__moz_xmalloc referenced in function "void * __cdecl operator new(unsigned int)" (??2@YAPAXI@Z) xpcomglue_staticruntime.lib(nsCOMArray.obj) : error LNK2019: unresolved external symbol __imp__moz_free referenced in function "void __cdecl operator delete(void *)" (??3@YAXPAX@Z) webapprt-stub.exe : fatal error LNK1120: 2 unresolved externals

(In reply to comment #5) > Backed out Try using nsMemory::Clone and nsMemory::Free perhaps?

After discussing with bsmedberg, I opted for NS_Alloc and NS_Free. This went green on try, so I repushed the changeset: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d31ac32628e