[llvm-commits] [llvm] r157546 - /llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h

[Benjamin Kramer]

On 29.05.2012, at 01:28, Richard Smith wrote:

> On Sun, May 27, 2012 at 1:46 PM, Benjamin Kramer <benny.kra at googlemail.com> wrote:
> Author: d0k
> Date: Sun May 27 15:46:04 2012
> New Revision: 157546
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=157546&view=rev
> Log:
> IntrusiveRefCntPtr: Use the same pattern as the other operator= overloads when using rvalue refs.
> 
> Modified:
>    llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h
> 
> Modified: llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h?rev=157546&r1=157545&r2=157546&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h (original)
> +++ llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h Sun May 27 15:46:04 2012
> @@ -23,6 +23,7 @@
> 
>  #include "llvm/Support/Casting.h"
>  #include "llvm/Support/Compiler.h"
> +#include <memory>
> 
>  namespace llvm {
> 
> @@ -146,15 +147,13 @@
> 
>  #if LLVM_USE_RVALUE_REFERENCES
>     IntrusiveRefCntPtr& operator=(IntrusiveRefCntPtr&& S) {
> -      Obj = S.Obj;
> -      S.Obj = 0;
> +      this_type(std::move(S)).swap(*this);
>       return *this;
>     }
> 
>     template <class X>
>     IntrusiveRefCntPtr& operator=(IntrusiveRefCntPtr<X>&& S) {
> -      Obj = S.getPtr();
> -      S.Obj = 0;
> +      this_type(std::move(S)).swap(*this);
>       return *this;
>     }
> 
> How about replacing all five operator= overloads (duplicating the work of the constructors) and the 'replace' member function with just:
> 
> IntrusiveRefCntPtr& operator=(IntrusiveRefCntPtr O) {
>   swap(O);
>   return *this;
> }
> 
> As well as simplifying the code, this would give us move semantics in some cases in C++98.

The current design of operator= is basically copied from c++11's std::shared_ptr, my c++-fu is too weak to prove that this fully is equivalent. The whole point of the moving RefCntPtrs is avoiding unnecessary retain/releases because it requires dereferencing the pointer which can be expensive. Doesn't your solution add an extra retain/release in the move case? At the moment it should only release the current pointer and move in the new value.

- Ben