Closed Bug 178187 Opened 22 years ago Closed 13 years ago

|nsCOMPtr|: better named functions for using |already_AddRefed| with raw pointers

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: scc, Unassigned)

References

Details

see bug #172030 for a complete discussion, but I will abstract here: When a function returns an |already_AddRefed| and the caller intends to manage that result in a raw pointer, the current notation is unclear, and may be confusing to people who have formed a specific mental model about the meaning of |.get()| from other classes T* t = f().get() ...does not make clear the transfer of ownership (though please note, |already_AddRefed| is an annotation, not a manager of ownership). Maybe |already_AddRefed<T>::get()| needs a new name. Maybe we need a new external helper function. I currently favor a name like |accept_ownership_of|: T* t = accept_ownership_of( f() ); Note: the use of raw pointers in this position is actively discouraged. The use is expected to be rare. Therefor, the more explicit the naming the better, and also this bug is probably of significantly lower priority.
Just as a note... I've been going through the tree looking for functions that return raw addrefed pointers. About 50-60% of the callers of such functions are actually assigning the result directly into an out param. So if we were to start converting functions returning addrefed pointers to returning already_AddRefed (a not-too-small "if") we would be hard=pressed to discourage the use (unless we demand that people use an intermediate nsCOMPtr with the ensuing AddRef/Release). I agree that the annotation should be very explicit in such cases; I'm just not sure I agree that this is very low priority. ;)
Status: NEW → ASSIGNED
Noted. That's a good observation.
Severity: normal → enhancement
Here's one possible way to add this machinery. In this scheme, we go to a little trouble to prevent accidents in cases like already_AddRefed<T> CreateAFoo(); // this is the right way to assign into an |nsCOMPtr| nsCOMPtr<T> t = CreateAFoo(); // but with the scheme below, this works too nsCOMPtr<T> t = accept_ownership_of( CreateAFoo() ); nsCOMPtr<T> t = CreateAFoo().get(); Note that with the curent implementation of |already_AddRefed::get()|, this last statement would be a leak. // add this new class template <class T> struct unsafe_already_AddRefed { unsafe_already_AddRefed( T* aRawPtr ) : mRawPtr(aRawPtr) { // nothing else to do here } operator T*() const { return mRawPtr; } // this is the operation that makes // this class `unsafe', it automatically // converts to a raw pointer on demand // throwing away the extra type information T* mRawPtr; }; // change this existing class (in the |get| member) template <class T> struct already_AddRefed { already_AddRefed( T* aRawPtr ) : mRawPtr(aRawPtr) { // nothing else to do here } unsafe_already_AddRefed<T> get() const { return unsafe_already_AddRefed<T>(mRawPtr); } T* mRawPtr; }; // this is the whole point, adding this utility template <class T> inline const unsafe_already_AddRefed<T> accept_ownership_of( const already_AddRefed<T>& aAlreadyAddRefedPtr ) { return aAlreadyAddRefedPtr.get(); } // add some more signatures of existing functions for the // new `unsafe' class template <class T> inline const already_AddRefed<T> getter_AddRefs( const unsafe_already_AddRefed<T>& aAlreadyAddRefedPtr ) { return already_AddRefed<T>(aAlreadyAddRefedPtr); } template <class T> inline const already_AddRefed<T> dont_AddRef( const unsafe_already_AddRefed<T>& aAlreadyAddRefedPtr ) { return already_AddRefed<T>(aAlreadyAddRefedPtr); } template <class T> inline void do_QueryInterface( const unsafe_already_AddRefed<T>& ) { // This signature exists... blah, blah, // as per the other dummy |do_QueryInterface| implementations } template <class T> inline void do_QueryInterface( const unsafe_already_AddRefed<T>&, nsresult* ) { // This signature exists... blah, blah, // as per the other dummy |do_QueryInterface| implementations } // a couple of additions to |nsCOMPtr| itself template <class T> inline nsCOMPtr<T>::nsCOMPtr( const unsafe_already_AddRefed<T>& aSmartPtr ) : NSCAP_CTOR_BASE(aSmartPtr.mRawPtr) { NSCAP_LOG_ASSIGNMENT(this, aSmartPtr.mRawPtr); NSCAP_ASSERT_NO_QUERY_NEEDED(); } template <class T> inline nsCOMPtr<T>& nsCOMPtr<T>::operator=( const unsafe_already_AddRefed<T>& rhs ) { assign_assuming_AddRef(rhs.mRawPtr); NSCAP_ASSERT_NO_QUERY_NEEDED(); } // ...and similarly for |nsCOMPtr<nsISupports>|
Not sure I like the above solution. I don't like the idea of adding yet another support class. I definitely would want to examine generated code to see how the compilers deal with this. You can see how this could quite easily suck in practice, or, on the other hand, be completely optimized out of the generated code. Also, on some broken compilers, I would expect assignment into an |nsCOMPtr| to be ambiguous trying to choose between the unconverted class, and assignment from the automatically converted raw pointer. This also needs to be tested.
Assignee: scc → nobody
Status: ASSIGNED → NEW
QA Contact: scc → xpcom
forget/swap to the rescue
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.