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

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Description

[Scott Collins]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 2

[Scott Collins]

Noted.  That's a good observation.

Comment 3

[Scott Collins]

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>|

Comment 4

[Scott Collins]

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.

Comment 5

[Benjamin Smedberg]

forget/swap to the rescue