It would be nice if there were a way to transfer ownership from one nsCOMPtr to another. This would allow one to use nsCOMPtrs in tight loops where they would otherwise impose extra refcounting costs. Such a feature would allow the transfer of a refcount from one nsCOMPtr to another (releasing the second beforehand and nulling the first afterwards) I came up with two possible implementations of such a feature (using the same syntax). They compile to the exact same result on gcc2.96 with -O2 -- however, I'm not sure that result is ideal. One of the two implementations (#2, the one that's slightly more elegant, of course) would probably break VC 4.2 judging by the comments in nsCOMPtr.h. These implementations would support the syntax |SmartPtr1 = transfer_Ownership(SmartPtr2);|. I have done no runtime tests of these whatsoever -- I have no idea if they're really right.

How about |nsCOMPtr<T>::swap( nsCOMPtr<T>& )|? Give me an example loop where you want to exploit this functionality.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsCSSStyleSheet.cpp&rev=3.162&mark=3285#3275

Here is the definition of the member function |swap|. void swap( nsCOMPtr<T>& rhs ) { T* temp = rhs.mRawPtr; rhs.mRawPtr = mRawPtr; mRawPtr = temp; } In use, it is assumed that |rhs| is probably a |nsCOMPtr| that is about to go away. It will then be the reference that |Release|s our old referent. Thus, it has exactly the same overhead as your solution. |swap| is a well understood idea in the standard libraries and I think could be pretty applicable to this situation. nsCOMPtr<nsIFoo> cur = ...; for ( ...; cur; ... ) { nsCOMPtr<nsIFoo> child; cur->GetChild( getter_AddRefs(child) ); cur.swap(child); } ...would this meet your needs/expectations?

based on the URL you give above, |swap| might be just the thing

probably need to worry about some logging magic to make this work appropriately, no matter which solution we pick

David, does my patch above do the right thing for logging? Or do we need, e.g., |NSCAP_LOG_SWAP|? Some dissassembly would be good to ensure that this optimization really does what we want it to, and that when unused, the work to return a value is optimized away as I state in the comment. I'll get around to that. In the meanwhile, how do you guys feel about this patch?

For the logging, I think you should have an NSCAP_LOG_RELEASE for each in addition to the NSCAP_LOG_ASSIGNMENT. (They really indicate when the nsCOMPtr gains or loses ownership of a reference, but previously all cases for the latter were when it called |Release|).

hmmm, then won't I need to |NSCAP_LOG_ADDREF| as well? And by the way, neither of those names appear in "nsCOMPtr.h". Will I need to do the same magic for them that I do for |NSCAP_LOG_ASSIGNMENT|? It is assumed that logging clients of "nsCOMPtr.h" appropriately define |NSCAP_ADDREF| and |NSCAP_RELEASE|, as per the comment starting near line #113.

Oops... yeah, we don't *have* NSCAP_LOG_RELEASE yet. So I think the logging is messy. I'll try to look at it on the weekend, or sometime...

Does this seem like a reasonable solution? If so, I'll test it. I don't want to inflict multiple full rebuilds on myself for a solution that I'll have to change in review... :-)

adjusting summary and collecting under tracking bug # 178174

|swap| with another |nsCOMPtr| _and_ |swap| with a raw pointer are both useful. See bug #190746

I really think this has become the most important planned change to |nsCOMPtr|.

*** Bug 190746 has been marked as a duplicate of this bug. ***

Adding keyword 'perf' as per bug #190746 which was marked a duplicate of this bug. See that bug for dbradley's patch illuminating uses.

Here's the last patch updated to the tip, additional signatures, and void returns. I'm not sure the logging is correct for the raw pointer case. dbaron?

Comment on attachment 113064 [details] [diff] [review] updated to the tip; |void| return; also |swap| with raw pointers dbaron, would you review?

r=dbradley For all but the logging code. I think this could be a pretty decent perf win, if you combine the reduction of addref/release pairs in the return pattern with what we would get in loops that could benefit as well from swap.

Comment on attachment 113064 [details] [diff] [review] updated to the tip; |void| return; also |swap| with raw pointers I think the logging here isn't going to work. I think the separation of NSCAP_RELEASE from the logging of release that I did in attachment 48042 [details] [diff] [review] is needed.

Comment on attachment 113064 [details] [diff] [review] updated to the tip; |void| return; also |swap| with raw pointers > ... can save a pair of recount operations i think you meant reFcount :)

I fixed the logging as per the older attachment cited by dbaron, however, I'm not sure this is right for the `|swap| with raw pointer' case.

Comment on attachment 113505 [details] [diff] [review] attempt to fix logging, fix typo pointed out by timeless requesting reviews

Comment on attachment 113505 [details] [diff] [review] attempt to fix logging, fix typo pointed out by timeless >+ void >+ swap( T*& rhs ) >+ // ...exchange ownership with |rhs|; can save a pair of refcount operations >+ { >+#ifdef NSCAP_FEATURE_DEBUG_PTR_TYPES >+ T* temp = rhs; >+#else >+ nsISupports* temp = rhs; >+#endif drop this one: >+ NSCAP_LOG_ASSIGNMENT(&rhs, mRawPtr); keep these two: >+ NSCAP_LOG_ASSIGNMENT(this, temp); >+ NSCAP_LOG_RELEASE(this, mRawPtr); and drop this one: >+ NSCAP_LOG_RELEASE(&ths, temp); >+ rhs = mRawPtr; >+ mRawPtr = temp; >+ NSCAP_ASSERT_NO_QUERY_NEEDED(); Other than that, r= or sr= dbaron.

fixed logging as per dbaron's review above. fixed another typo.

Comment on attachment 113510 [details] [diff] [review] fixed logging and another typo fixed as per dbaron's recommendations; setting flags to allow him to forward his sr to this patch

Comment on attachment 113510 [details] [diff] [review] fixed logging and another typo requesting approval

Comment on attachment 113510 [details] [diff] [review] fixed logging and another typo this can wait for 1.4a

A few quick questions before r=, 1. I notice the presence of NSCAP macros in the CPP file, these won't be defined the same as they are for the header, if the code including the header has its own versions. This isn't just a problem for thise patch, though. 2. Something in me keeps wanting to see swap( nsCOMPtr<T>& rhs) implemented in terms of swap(T*& rhs). Not a lot of logic there, outside of logging, and logging would probably be the more difficult thing to refactor. 3. The comment: The following three macros (|NSCAP_ADDREF|, |NSCAP_RELEASE|, and |NSCAP_LOG_ASSIGNMENT|) probably should be adjusted to refect NSCAP_LOG_RELEASE.

I actually did fix up the macro definitions mentioned by dbradley. I just didn't diff high enough to show that in my patch. Sorry. This patch is identical to the previous one, except that it also shows the changes to "nsTraceRefcnt.h"

Comment on attachment 113816 [details] [diff] [review] didn't perform my diff from a high enough directory... r=dbradley I still think item 1 above is an issue, but I don't think it's something that should block this patch.

patch checked into the trunk

Another pattern: NS_ADDREF(*anOutParam = thensCOMPtr); Can/should we count on the out param being set to null before the call?

Oh, that would just become: *anOutParam = thensCOMPtr; thennsCOMPtr.swap(nsnull);