commit fffda1d36867d2a7f40fe77bc5410d16b7654a5b
author Tom Sepez <tsepez@chromium.org> Wed Aug 31 22:40:07 2022 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Aug 31 22:40:07 2022 +0000
tree 1176c6a8b42c338a4dd525757350c40a903ea8e7
parent b14660cb0e393777cd0007437cfb79a4e9e54f52

Avoid ref-churn when move conversion constructing RetainPtrs

Prior to this, move constructing from a different, but implicitly
convertible RetainPtr type would invoke a copy, as shown by the
new test.

-- add test for copy conversion constructor.

Change-Id: I9cd0e88eb143efa793d09d5e50ec559df92bb21f
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/97250
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

core/fxcrt/retain_ptr.h

diff --git a/core/fxcrt/retain_ptr.h b/core/fxcrt/retain_ptr.h
index 87e22f7..4087dbb 100644
--- a/core/fxcrt/retain_ptr.h
+++ b/core/fxcrt/retain_ptr.h
@@ -32,18 +32,29 @@
   }
 
   RetainPtr() = default;
+
+  // Copy-construct a RetainPtr.
+  // Required in addition to copy conversion constructor below.
   RetainPtr(const RetainPtr& that) : RetainPtr(that.Get()) {}
 
   // Move-construct a RetainPtr. After construction, |that| will be NULL.
-  RetainPtr(RetainPtr&& that) noexcept { Swap(that); }
+  // Required in addition to move conversion constructor below.
+  RetainPtr(RetainPtr&& that) noexcept { Unleak(that.Leak()); }
 
   // Deliberately implicit to allow returning nullptrs.
   // NOLINTNEXTLINE(runtime/explicit)
   RetainPtr(std::nullptr_t ptr) {}
 
+  // Copy conversion constructor.
   template <class U>
   RetainPtr(const RetainPtr<U>& that) : RetainPtr(that.Get()) {}
 
+  // Move-conversion constructor.
+  template <class U>
+  RetainPtr(RetainPtr<U>&& that) noexcept {
+    Unleak(that.Leak());
+  }
+
   template <class U>
   RetainPtr<U> As() const {
     return RetainPtr<U>(static_cast<U*>(Get()));
@@ -127,6 +138,9 @@
   Retainable(const Retainable& that) = delete;
   Retainable& operator=(const Retainable& that) = delete;
 
+  // These need to be const methods operating on a mutable member so that
+  // RetainPtr<const T> can be used for an object that is otherwise const
+  // apart from the internal ref-counting.
   void Retain() const { ++m_nRefCount; }
   void Release() const {
     DCHECK(m_nRefCount > 0);

core/fxcrt/retain_ptr_unittest.cpp

diff --git a/core/fxcrt/retain_ptr_unittest.cpp b/core/fxcrt/retain_ptr_unittest.cpp
index 20c597d..655a964 100644
--- a/core/fxcrt/retain_ptr_unittest.cpp
+++ b/core/fxcrt/retain_ptr_unittest.cpp
@@ -45,6 +45,22 @@
   EXPECT_EQ(2, obj.release_count());
 }
 
+TEST(RetainPtr, CopyConversionCtor) {
+  PseudoRetainable obj;
+  {
+    RetainPtr<PseudoRetainable> ptr1(&obj);
+    {
+      RetainPtr<const PseudoRetainable> ptr2(ptr1);
+      EXPECT_EQ(2, obj.retain_count());
+      EXPECT_EQ(0, obj.release_count());
+    }
+    EXPECT_EQ(2, obj.retain_count());
+    EXPECT_EQ(1, obj.release_count());
+  }
+  EXPECT_EQ(2, obj.retain_count());
+  EXPECT_EQ(2, obj.release_count());
+}
+
 TEST(RetainPtr, MoveCtor) {
   PseudoRetainable obj;
   {
@@ -63,6 +79,24 @@
   EXPECT_EQ(1, obj.release_count());
 }
 
+TEST(RetainPtr, MoveConversionCtor) {
+  PseudoRetainable obj;
+  {
+    RetainPtr<PseudoRetainable> ptr1(&obj);
+    {
+      RetainPtr<const PseudoRetainable> ptr2(std::move(ptr1));
+      EXPECT_FALSE(ptr1.Get());
+      EXPECT_EQ(&obj, ptr2.Get());
+      EXPECT_EQ(1, obj.retain_count());
+      EXPECT_EQ(0, obj.release_count());
+    }
+    EXPECT_EQ(1, obj.retain_count());
+    EXPECT_EQ(1, obj.release_count());
+  }
+  EXPECT_EQ(1, obj.retain_count());
+  EXPECT_EQ(1, obj.release_count());
+}
+
 TEST(RetainPtr, ResetNull) {
   PseudoRetainable obj;
   {

testing/pseudo_retainable.h

diff --git a/testing/pseudo_retainable.h b/testing/pseudo_retainable.h
index c4390d6..45f2b04 100644
--- a/testing/pseudo_retainable.h
+++ b/testing/pseudo_retainable.h
@@ -8,8 +8,8 @@
 class PseudoRetainable {
  public:
   PseudoRetainable() = default;
-  void Retain() { ++retain_count_; }
-  void Release() {
+  void Retain() const { ++retain_count_; }
+  void Release() const {
     if (++release_count_ == retain_count_)
       alive_ = false;
   }
@@ -18,9 +18,9 @@
   int release_count() const { return release_count_; }
 
  private:
-  bool alive_ = true;
-  int retain_count_ = 0;
-  int release_count_ = 0;
+  mutable bool alive_ = true;
+  mutable int retain_count_ = 0;
+  mutable int release_count_ = 0;
 };
 
 #endif  // TESTING_PSEUDO_RETAINABLE_H_