Closed Bug 392493 Opened 17 years ago Closed 17 years ago

Add forget() method to nsCOMPtr and nsRefPtr

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch, v1 (obsolete) — 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)
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.
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+
Comment on attachment 277030 [details] [diff] [review] Patch, v2 [checked in] a=bzbarsky
Attachment #277030 - Flags: approval1.9? → approval1.9+
Attached patch Tests (obsolete) — Splinter Review
Attachment #278619 - Flags: review?(dbaron)
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)
Attachment #277030 - Attachment description: Patch, v2 → Patch, v2 [checked in]
Comment on attachment 277030 [details] [diff] [review] Patch, v2 [checked in] Fixed on trunk.
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+
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]
Landed tests.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: