Closed Bug 392493 Opened 12 years ago Closed 12 years ago
Add forget() method to ns
COMPtr and ns Ref Ptr
I've been writing methods with the following signature: already_AddRefed<nsIFoo> bar(); In such methods I'm usually holding my to-be-returned value in an nsCOMPtr or nsRefPtr until the very end of the function. Returning the value of those smart pointers without unnecessary AddRef/Release calls is awkward: nsCOMPtr<nsIFoo> toBeReturned = do_CreateInstance(...); nsIFoo* outVal = nsnull; toBeReturned.swap(outVal); return outVal; It would be much nicer if we had nsAutoPtr's forget() capability for methods like this. Then we could just write: nsCOMPtr<nsIFoo> toBeReturned = do_CreateInstance(...); return toBeReturned.forget(); Thoughts? Attaching a patch that implements forget() for both nsCOMPtr and nsRefPtr. The NSCAP macros confused me a little so hopefully I got those right.
Oh, andd bz suggested fixing already_AddRefed to take an nsCOMPtr and do the swap-with-null automatically but I think that would be too "magic" and people would be confused by reading the code. But that's certainly another option.
Would it make sense to make forget() return an already_AddRefed<T> to start with?
Hm, yeah. This is better. And as bz suggested I've changed the internals to just use the already implemented swap methods.
Comment on attachment 277030 [details] [diff] [review] Patch, v2 [checked in] r=dbaron, although you might also want to add a test to TestCOMPtr.cpp and TestAutoPtr.cpp if they still compile and run...
Attachment #277030 - Flags: review?(dbaron) → review+
Attachment #277030 - Flags: approval1.9?
Comment on attachment 277030 [details] [diff] [review] Patch, v2 [checked in] a=bzbarsky
Attachment #277030 - Flags: approval1.9? → approval1.9+
I had a typo and I was using IBar when all I really wanted was IFoo.
Attachment #277030 - Attachment description: Patch, v2 → Patch, v2 [checked in]
Resolving, but still want to get those tests in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 278622 [details] [diff] [review] Tests [checked in] r=dbaron. Sorry for the delay. I suppose non-automated is better than nothing, and we'd probably break something else if we actually broke this...
Attachment #278622 - Flags: review?(dbaron) → review+
Why can't this be automated, again? Seems like it would be fairly simple, really -- I'd just ignore the existing tests, actually, and start a new one (cf. the new xpcom/tests/TestPipe.cpp versus xpcom/tests/TestPipes.cpp). I don't really feel like the old tests are all that useful if they're not going to be run regularly, to be honest.
Attachment #278622 - Attachment description: Tests → Tests [checked in]
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.