Closed
Bug 450881
Opened 16 years ago
Closed 11 years ago
nsCOMArray: Helpers for XPIDL-based arrays? [array, size_is(count)] in/out nsIFoo fooObjects]
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: WeirdAl, Assigned: jcranmer)
Details
Attachments
(1 file)
4.81 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
<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.
Reporter | ||
Comment 1•16 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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.
Attachment #8362735 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 4•11 years ago
|
||
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
Comment 6•11 years ago
|
||
(In reply to comment #5)
> Backed out
Try using nsMemory::Clone and nsMemory::Free perhaps?
Assignee | ||
Comment 7•11 years ago
|
||
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
Comment 8•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•