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

[Richard Smith]

On Tue, May 29, 2012 at 6:17 AM, Benjamin Kramer
<benny.kra at googlemail.com>wrote:

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


No, it performs the same constructions and destructions. Your formulation
move-constructs and destroys an IntrusiveRefCntPtr object in the
"this_type(std::move(S))"
expression, whereas mine does the same when constructing the "O" parameter.

[My approach has a tiny advantage if the construction of "O" can be elided:
in that case, it's one move-constructor call and one no-op destructor call
cheaper, but both of those calls should be trivial to optimize away, so I
don't think that's significant.]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120530/a3eb6ff9/attachment.html>