Closed
Bug 392493
Opened 17 years ago
Closed 17 years ago
Add forget() method to nsCOMPtr and nsRefPtr
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: bent.mozilla, Assigned: bent.mozilla)
Details
Attachments
(2 files, 2 obsolete files)
2.21 KB,
patch
|
dbaron
:
review+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
2.58 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #277002 -
Flags: review?(dbaron)
Assignee | ||
Comment 1•17 years ago
|
||
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.
![]() |
||
Comment 2•17 years ago
|
||
Would it make sense to make forget() return an already_AddRefed<T> to start with?
Assignee | ||
Comment 3•17 years ago
|
||
Hm, yeah. This is better. And as bz suggested I've changed the internals to just use the already implemented swap methods.
Assignee: nobody → bent.mozilla
Attachment #277002 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #277030 -
Flags: review?(dbaron)
Attachment #277002 -
Flags: review?(dbaron)
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+
Assignee | ||
Updated•17 years ago
|
Attachment #277030 -
Flags: approval1.9?
![]() |
||
Comment 5•17 years ago
|
||
Comment on attachment 277030 [details] [diff] [review]
Patch, v2 [checked in]
a=bzbarsky
Attachment #277030 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 6•17 years ago
|
||
Attachment #278619 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•17 years ago
|
||
I had a typo and I was using IBar when all I really wanted was IFoo.
Attachment #278619 -
Attachment is obsolete: true
Attachment #278622 -
Flags: review?(dbaron)
Attachment #278619 -
Flags: review?(dbaron)
Assignee | ||
Updated•17 years ago
|
Attachment #277030 -
Attachment description: Patch, v2 → Patch, v2 [checked in]
Assignee | ||
Comment 8•17 years ago
|
||
Comment on attachment 277030 [details] [diff] [review]
Patch, v2 [checked in]
Fixed on trunk.
Assignee | ||
Comment 9•17 years ago
|
||
Resolving, but still want to get those tests in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
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+
Comment 11•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Attachment #278622 -
Attachment description: Tests → Tests [checked in]
You need to log in
before you can comment on or make changes to this bug.
Description
•