Closed Bug 392493 Opened 12 years ago Closed 12 years ago

Add forget() method to nsCOMPtr and nsRefPtr

Categories

(Core :: XPCOM, defect)

defect
Not set

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: 12 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.