Unified scoped_refptr's assignment operators.

Replaced several of scoped_refptr's assignment operators by a unified assignment
operator. This works beautifully in tandem with scoped_refptr's different
constructors, and it removes some duplicated and tricky code. It is exception
safe, and works fine in case of self assignment.

We still need to keep operator=(T*), despite the implicit conversion constructor
scoped_refptr(T*). There is code assigning rtc::scoped_refptr<T> to
scoped_refptr<T>, and we want to keep this code compiling and working as is.
rtc::scoped_refptr<T> has an implicit conversion operator to T*. C++ allows one
implicit conversion, but not two, to be chained in a row.

Marked scoped_refptr's move and swap operations as noexcept. This allows
std::vector<scoped_refptr<T>> to use scoped_refptr's more efficient move
operations instead of copy operations. This avoids extraneous calls to
T::AddRef() and T::Release() when vector is reallocating its internal buffer.

Change-Id: Id6f360f8acab827133fe5bef829a7a2e308b87fd
Reviewed-on: https://chromium-review.googlesource.com/802338Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524008}

base/memory/ref_counted_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "base/memory/ref_counted.h"
 
+#include <type_traits>
 #include <utility>
 
 #include "base/test/gtest_util.h"
@@ -161,6 +162,12 @@ TEST(RefCountedUnitTest, TestSelfAssignment) {
   scoped_refptr<SelfAssign> var(p);
   var = var;
   EXPECT_EQ(var.get(), p);
+  var = std::move(var);
+  EXPECT_EQ(var.get(), p);
+  var.swap(var);
+  EXPECT_EQ(var.get(), p);
+  swap(var, var);
+  EXPECT_EQ(var.get(), p);
 }
 
 TEST(RefCountedUnitTest, ScopedRefPtrMemberAccess) {
@@ -563,21 +570,21 @@ TEST(RefCountedUnitTest, MoveConstructorDerived) {
 }
 
 TEST(RefCountedUnitTest, TestOverloadResolutionCopy) {
-  scoped_refptr<Derived> derived(new Derived);
-  scoped_refptr<SelfAssign> expected(derived);
+  const scoped_refptr<Derived> derived(new Derived);
+  const scoped_refptr<SelfAssign> expected(derived);
   EXPECT_EQ(expected, Overloaded(derived));
 
-  scoped_refptr<Other> other(new Other);
+  const scoped_refptr<Other> other(new Other);
   EXPECT_EQ(other, Overloaded(other));
 }
 
 TEST(RefCountedUnitTest, TestOverloadResolutionMove) {
   scoped_refptr<Derived> derived(new Derived);
-  scoped_refptr<SelfAssign> expected(derived);
+  const scoped_refptr<SelfAssign> expected(derived);
   EXPECT_EQ(expected, Overloaded(std::move(derived)));
 
   scoped_refptr<Other> other(new Other);
-  scoped_refptr<Other> other2(other);
+  const scoped_refptr<Other> other2(other);
   EXPECT_EQ(other2, Overloaded(std::move(other)));
 }
 

base/memory/scoped_refptr.h

@@ -9,6 +9,7 @@
 
 #include <iosfwd>
 #include <type_traits>
+#include <utility>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
@@ -164,7 +165,7 @@ class scoped_refptr {
   }
 
   // Copy constructor.
-  scoped_refptr(const scoped_refptr<T>& r) : ptr_(r.ptr_) {
+  scoped_refptr(const scoped_refptr& r) : ptr_(r.ptr_) {
     if (ptr_)
       AddRef(ptr_);
   }
@@ -180,13 +181,13 @@ class scoped_refptr {
 
   // Move constructor. This is required in addition to the conversion
   // constructor below in order for clang to warn about pessimizing moves.
-  scoped_refptr(scoped_refptr&& r) : ptr_(r.get()) { r.ptr_ = nullptr; }
+  scoped_refptr(scoped_refptr&& r) noexcept : ptr_(r.ptr_) { r.ptr_ = nullptr; }
 
   // Move conversion constructor.
   template <typename U,
             typename = typename std::enable_if<
                 std::is_convertible<U*, T*>::value>::type>
-  scoped_refptr(scoped_refptr<U>&& r) : ptr_(r.get()) {
+  scoped_refptr(scoped_refptr<U>&& r) noexcept : ptr_(r.ptr_) {
     r.ptr_ = nullptr;
   }
 
@@ -212,48 +213,15 @@ class scoped_refptr {
     return ptr_;
   }
 
-  scoped_refptr<T>& operator=(T* p) {
-    // AddRef first so that self assignment should work
-    if (p)
-      AddRef(p);
-    T* old_ptr = ptr_;
-    ptr_ = p;
-    if (old_ptr)
-      Release(old_ptr);
-    return *this;
-  }
-
-  scoped_refptr<T>& operator=(const scoped_refptr<T>& r) {
-    return *this = r.ptr_;
-  }
-
-  template <typename U>
-  scoped_refptr<T>& operator=(const scoped_refptr<U>& r) {
-    return *this = r.get();
-  }
-
-  scoped_refptr<T>& operator=(scoped_refptr<T>&& r) {
-    scoped_refptr<T> tmp(std::move(r));
-    tmp.swap(*this);
-    return *this;
-  }
+  scoped_refptr& operator=(T* p) { return *this = scoped_refptr(p); }
 
-  template <typename U>
-  scoped_refptr<T>& operator=(scoped_refptr<U>&& r) {
-    // We swap with a temporary variable to guarantee that |ptr_| is released
-    // immediately. A naive implementation which swaps |this| and |r| would
-    // unintentionally extend the lifetime of |ptr_| to at least the lifetime of
-    // |r|.
-    scoped_refptr<T> tmp(std::move(r));
-    tmp.swap(*this);
+  // Unified assignment operator.
+  scoped_refptr& operator=(scoped_refptr r) noexcept {
+    swap(r);
     return *this;
   }
 
-  void swap(scoped_refptr<T>& r) {
-    T* tmp = ptr_;
-    ptr_ = r.ptr_;
-    r.ptr_ = tmp;
-  }
+  void swap(scoped_refptr& r) noexcept { std::swap(ptr_, r.ptr_); }
 
   explicit operator bool() const { return ptr_ != nullptr; }
 
@@ -351,7 +319,7 @@ std::ostream& operator<<(std::ostream& out, const scoped_refptr<T>& p) {
 }
 
 template <typename T>
-void swap(scoped_refptr<T>& lhs, scoped_refptr<T>& rhs) {
+void swap(scoped_refptr<T>& lhs, scoped_refptr<T>& rhs) noexcept {
   lhs.swap(rhs);
 }