Fix unowned/retain ptr conversion and assignment.
Define all the possible ctor/assignment possibilities rather than
letting the compiler deduce anything.
-- Make tests consistent with each other.
Change-Id: I7ed7f747d283b3d3e1394c84921d38867e76c13b
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/98455
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fxcrt/retain_ptr.h b/core/fxcrt/retain_ptr.h
index eda9517..ccf3615 100644
--- a/core/fxcrt/retain_ptr.h
+++ b/core/fxcrt/retain_ptr.h
@@ -60,7 +60,20 @@
Unleak(that.Leak());
}
+ // Assign a RetainPtr from nullptr;
+ RetainPtr& operator=(std::nullptr_t) noexcept {
+ Reset();
+ return *this;
+ }
+
+ // Assign a RetainPtr from a raw ptr.
+ RetainPtr& operator=(T* that) noexcept {
+ Reset(that);
+ return *this;
+ }
+
// Copy-assign a RetainPtr.
+ // Required in addition to copy conversion assignment below.
RetainPtr& operator=(const RetainPtr& that) {
if (*this != that)
Reset(that.Get());
@@ -68,11 +81,31 @@
}
// Move-assign a RetainPtr. After assignment, |that| will be NULL.
+ // Required in addition to move conversion assignment below.
RetainPtr& operator=(RetainPtr&& that) noexcept {
Unleak(that.Leak());
return *this;
}
+ // Copy-convert assign a RetainPtr.
+ template <class U,
+ typename = typename std::enable_if<
+ std::is_convertible<U*, T*>::value>::type>
+ RetainPtr& operator=(const RetainPtr<U>& that) {
+ if (*this != that)
+ Reset(that.Get());
+ return *this;
+ }
+
+ // Move-convert assign a RetainPtr. After assignment, |that| will be NULL.
+ template <class U,
+ typename = typename std::enable_if<
+ std::is_convertible<U*, T*>::value>::type>
+ RetainPtr& operator=(RetainPtr<U>&& that) noexcept {
+ Unleak(that.Leak());
+ return *this;
+ }
+
~RetainPtr() = default;
template <class U>
diff --git a/core/fxcrt/retain_ptr_unittest.cpp b/core/fxcrt/retain_ptr_unittest.cpp
index 2fdd993..2312e3f 100644
--- a/core/fxcrt/retain_ptr_unittest.cpp
+++ b/core/fxcrt/retain_ptr_unittest.cpp
@@ -36,12 +36,17 @@
} // namespace
-TEST(RetainPtr, Null) {
+TEST(RetainPtr, DefaultCtor) {
RetainPtr<PseudoRetainable> ptr;
EXPECT_FALSE(ptr.Get());
}
-TEST(RetainPtr, Normal) {
+TEST(RetainPtr, NullptrCtor) {
+ RetainPtr<PseudoRetainable> ptr(nullptr);
+ EXPECT_FALSE(ptr.Get());
+}
+
+TEST(RetainPtr, RawCtor) {
PseudoRetainable obj;
{
RetainPtr<PseudoRetainable> ptr(&obj);
@@ -69,22 +74,6 @@
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;
{
@@ -103,6 +92,22 @@
EXPECT_EQ(1, 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, MoveConversionCtor) {
PseudoRetainable obj;
{
@@ -121,6 +126,108 @@
EXPECT_EQ(1, obj.release_count());
}
+TEST(RetainPtr, NullptrAssign) {
+ PseudoRetainable obj;
+ RetainPtr<PseudoRetainable> ptr(&obj);
+ ptr = nullptr;
+ EXPECT_FALSE(ptr);
+}
+
+TEST(RetainPtr, RawAssign) {
+ PseudoRetainable obj;
+ RetainPtr<PseudoRetainable> ptr;
+ ptr = &obj;
+ EXPECT_EQ(&obj, ptr);
+}
+
+TEST(RetainPtr, CopyAssign) {
+ PseudoRetainable obj;
+ {
+ RetainPtr<PseudoRetainable> ptr(&obj);
+ {
+ RetainPtr<PseudoRetainable> ptr2;
+ ptr2 = ptr;
+ EXPECT_EQ(2, obj.retain_count());
+ EXPECT_EQ(0, obj.release_count());
+ }
+ {
+ // Test assignment from wrapped underlying type.
+ RetainPtr<PseudoRetainable> ptr2;
+ ptr2 = pdfium::WrapRetain(ptr.Get());
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(1, obj.release_count());
+ }
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(2, obj.release_count());
+ }
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(3, obj.release_count());
+}
+
+TEST(RetainPtr, MoveAssign) {
+ PseudoRetainable obj;
+ {
+ RetainPtr<PseudoRetainable> ptr1(&obj);
+ {
+ RetainPtr<PseudoRetainable> ptr2;
+ EXPECT_EQ(&obj, ptr1.Get());
+ EXPECT_FALSE(ptr2.Get());
+ 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, CopyConvertAssign) {
+ PseudoRetainable obj;
+ {
+ RetainPtr<PseudoRetainable> ptr(&obj);
+ {
+ RetainPtr<const PseudoRetainable> ptr2;
+ ptr2 = ptr;
+ EXPECT_EQ(2, obj.retain_count());
+ EXPECT_EQ(0, obj.release_count());
+ }
+ {
+ // Test assignment from wrapped underlying type.
+ RetainPtr<const PseudoRetainable> ptr2;
+ ptr2 = pdfium::WrapRetain(ptr.Get());
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(1, obj.release_count());
+ }
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(2, obj.release_count());
+ }
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(3, obj.release_count());
+}
+
+TEST(RetainPtr, MoveConvertAssign) {
+ PseudoRetainable obj;
+ {
+ RetainPtr<PseudoRetainable> ptr1(&obj);
+ {
+ RetainPtr<const PseudoRetainable> ptr2;
+ ptr2 = std::move(ptr1);
+ EXPECT_FALSE(ptr1);
+ 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, AmbiguousExpression) {
class A : public Retainable {};
class B : public A {};
@@ -223,94 +330,6 @@
EXPECT_EQ(1, obj1.release_count());
}
-TEST(RetainPtr, Assign) {
- PseudoRetainable obj;
- {
- RetainPtr<PseudoRetainable> ptr(&obj);
- {
- RetainPtr<PseudoRetainable> ptr2;
- ptr2 = ptr;
- EXPECT_EQ(2, obj.retain_count());
- EXPECT_EQ(0, obj.release_count());
- }
- {
- // Test assignment from wrapped underlying type.
- RetainPtr<PseudoRetainable> ptr2;
- ptr2 = pdfium::WrapRetain(ptr.Get());
- EXPECT_EQ(3, obj.retain_count());
- EXPECT_EQ(1, obj.release_count());
- }
- EXPECT_EQ(3, obj.retain_count());
- EXPECT_EQ(2, obj.release_count());
- }
- EXPECT_EQ(3, obj.retain_count());
- EXPECT_EQ(3, obj.release_count());
-}
-
-TEST(RetainPtr, AssignConvert) {
- PseudoRetainable obj;
- {
- RetainPtr<PseudoRetainable> ptr(&obj);
- {
- RetainPtr<const PseudoRetainable> ptr2;
- ptr2 = ptr;
- EXPECT_EQ(2, obj.retain_count());
- EXPECT_EQ(0, obj.release_count());
- }
- {
- // Test assignment from wrapped underlying type.
- RetainPtr<const PseudoRetainable> ptr2;
- ptr2 = pdfium::WrapRetain(ptr.Get());
- EXPECT_EQ(3, obj.retain_count());
- EXPECT_EQ(1, obj.release_count());
- }
- EXPECT_EQ(3, obj.retain_count());
- EXPECT_EQ(2, obj.release_count());
- }
- EXPECT_EQ(3, obj.retain_count());
- EXPECT_EQ(3, obj.release_count());
-}
-
-TEST(RetainPtr, MoveAssign) {
- PseudoRetainable obj;
- {
- RetainPtr<PseudoRetainable> ptr1(&obj);
- {
- RetainPtr<PseudoRetainable> ptr2;
- EXPECT_EQ(&obj, ptr1.Get());
- EXPECT_FALSE(ptr2.Get());
- 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, MoveAssignConvert) {
- PseudoRetainable obj;
- {
- RetainPtr<PseudoRetainable> ptr1(&obj);
- {
- RetainPtr<const PseudoRetainable> ptr2;
- ptr2 = std::move(ptr1);
- EXPECT_FALSE(ptr1);
- 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, Equals) {
PseudoRetainable obj1;
PseudoRetainable obj2;
diff --git a/core/fxcrt/unowned_ptr.h b/core/fxcrt/unowned_ptr.h
index f9946f0..c506e2d 100644
--- a/core/fxcrt/unowned_ptr.h
+++ b/core/fxcrt/unowned_ptr.h
@@ -63,7 +63,7 @@
// Required in addition to move conversion constructor below.
constexpr UnownedPtr(UnownedPtr&& that) noexcept : m_pObj(that.Release()) {}
- // Copy conversion constructor.
+ // Copy-conversion constructor.
template <class U,
typename = typename std::enable_if<
std::is_convertible<U*, T*>::value>::type>
@@ -77,13 +77,20 @@
Reset(that.Release());
}
- // Assing an UnownedPtr from a raw ptr.
+ // Assign an UnownedPtr from nullptr.
+ UnownedPtr& operator=(std::nullptr_t) noexcept {
+ Reset();
+ return *this;
+ }
+
+ // Assign an UnownedPtr from a raw ptr.
UnownedPtr& operator=(T* that) noexcept {
Reset(that);
return *this;
}
// Copy-assign an UnownedPtr.
+ // Required in addition to copy conversion assignment below.
UnownedPtr& operator=(const UnownedPtr& that) noexcept {
if (*this != that)
Reset(that.Get());
@@ -91,12 +98,33 @@
}
// Move-assign an UnownedPtr. After assignment, |that| will be NULL.
+ // Required in addition to move conversion assignment below.
UnownedPtr& operator=(UnownedPtr&& that) noexcept {
if (*this != that)
Reset(that.Release());
return *this;
}
+ // Copy-convert assignment.
+ template <class U,
+ typename = typename std::enable_if<
+ std::is_convertible<U*, T*>::value>::type>
+ UnownedPtr& operator=(const UnownedPtr<U>& that) noexcept {
+ if (*this != that)
+ Reset(that.Get());
+ return *this;
+ }
+
+ // Move-convert assignment. After assignment, |that| will be NULL.
+ template <class U,
+ typename = typename std::enable_if<
+ std::is_convertible<U*, T*>::value>::type>
+ UnownedPtr& operator=(UnownedPtr<U>&& that) noexcept {
+ if (*this != that)
+ Reset(that.Release());
+ return *this;
+ }
+
~UnownedPtr() {
ProbeForLowSeverityLifetimeIssue();
m_pObj = nullptr;
diff --git a/core/fxcrt/unowned_ptr_unittest.cpp b/core/fxcrt/unowned_ptr_unittest.cpp
index 532d9c4..9807b75 100644
--- a/core/fxcrt/unowned_ptr_unittest.cpp
+++ b/core/fxcrt/unowned_ptr_unittest.cpp
@@ -64,11 +64,104 @@
} // namespace
-TEST(UnownedPtr, Null) {
+TEST(UnownedPtr, DefaultCtor) {
UnownedPtr<Clink> ptr;
EXPECT_FALSE(ptr.Get());
}
+TEST(UnownedPtr, NullptrCtor) {
+ UnownedPtr<Clink> ptr(nullptr);
+ EXPECT_FALSE(ptr.Get());
+}
+
+TEST(UnownedPtr, RawCtor) {
+ auto obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr(obj.get());
+ EXPECT_EQ(obj.get(), ptr.Get());
+}
+
+TEST(UnownedPtr, CopyCtor) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<Clink> ptr2(ptr1);
+ EXPECT_EQ(obj.get(), ptr2);
+ EXPECT_EQ(obj.get(), ptr1);
+}
+
+TEST(UnownedPtr, MoveCtor) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<Clink> ptr2(std::move(ptr1));
+ EXPECT_EQ(obj.get(), ptr2);
+ EXPECT_FALSE(ptr1);
+}
+
+TEST(UnownedPtr, CopyConversionCtor) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<const Clink> ptr2(ptr1);
+ EXPECT_EQ(obj.get(), ptr2);
+ EXPECT_EQ(obj.get(), ptr1);
+}
+
+TEST(UnownedPtr, MoveConversionCtor) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<const Clink> ptr2(std::move(ptr1));
+ EXPECT_EQ(obj.get(), ptr2);
+ EXPECT_FALSE(ptr1);
+}
+
+TEST(UnownedPtr, NullptrAssign) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr(obj.get());
+ ptr = nullptr;
+ EXPECT_FALSE(ptr);
+}
+
+TEST(UnownedPtr, RawAssign) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr;
+ ptr = obj.get();
+ EXPECT_EQ(obj.get(), ptr.Get());
+}
+
+TEST(UnownedPtr, CopyAssign) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<Clink> ptr2;
+ ptr2 = ptr1;
+ EXPECT_EQ(obj.get(), ptr1.Get());
+ EXPECT_EQ(obj.get(), ptr2.Get());
+}
+
+TEST(UnownedPtr, MoveAssign) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<Clink> ptr2;
+ ptr2 = std::move(ptr1);
+ EXPECT_FALSE(ptr1);
+ EXPECT_EQ(obj.get(), ptr2.Get());
+}
+
+TEST(UnownedPtr, CopyConversionAssign) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<const Clink> ptr2;
+ ptr2 = ptr1;
+ EXPECT_EQ(obj.get(), ptr1.Get());
+ EXPECT_EQ(obj.get(), ptr2.Get());
+}
+
+TEST(UnownedPtr, MoveConversionAssign) {
+ std::unique_ptr<Clink> obj = std::make_unique<Clink>();
+ UnownedPtr<Clink> ptr1(obj.get());
+ UnownedPtr<const Clink> ptr2;
+ ptr2 = std::move(ptr1);
+ EXPECT_FALSE(ptr1);
+ EXPECT_EQ(obj.get(), ptr2.Get());
+}
+
TEST(UnownedPtr, PtrOk) {
auto ptr1 = std::make_unique<Clink>();
{
@@ -85,22 +178,6 @@
#endif
}
-TEST(UnownedPtr, CopyCtor) {
- std::unique_ptr<Clink> obj = std::make_unique<Clink>();
- UnownedPtr<Clink> ptr1(obj.get());
- UnownedPtr<Clink> ptr2(ptr1);
- EXPECT_TRUE(ptr2);
- EXPECT_TRUE(ptr1);
-}
-
-TEST(UnownedPtr, CopyConversionCtor) {
- std::unique_ptr<Clink> obj = std::make_unique<Clink>();
- UnownedPtr<Clink> ptr1(obj.get());
- UnownedPtr<const Clink> ptr2(ptr1);
- EXPECT_TRUE(ptr2);
- EXPECT_TRUE(ptr1);
-}
-
TEST(UnownedPtr, ResetOk) {
auto ptr1 = std::make_unique<Clink>();
{
@@ -144,41 +221,6 @@
}
}
-TEST(UnownedPtr, MoveCtorOk) {
- UnownedPtr<Clink> outer;
- {
- auto owned = std::make_unique<Clink>();
- outer = owned.get();
- UnownedPtr<Clink> inner(std::move(outer));
- EXPECT_FALSE(outer.Get());
- EXPECT_EQ(owned.get(), inner.Get());
- }
-}
-
-TEST(UnownedPtr, MoveConversionCtorOk) {
- UnownedPtr<Clink> outer;
- {
- auto owned = std::make_unique<Clink>();
- outer = owned.get();
- UnownedPtr<const Clink> inner(std::move(outer));
- EXPECT_FALSE(outer.Get());
- }
-}
-
-TEST(UnownedPtr, MoveConversionAssignOk) {
- UnownedPtr<Clink> outer;
- {
- auto owned = std::make_unique<Clink>();
- outer = owned.get();
- // TODO(tsepez): disambiguate ctors and actually const-convert.
- // UnownedPtr<const Clink> inner;
- UnownedPtr<Clink> inner;
- inner = std::move(outer);
- EXPECT_FALSE(outer);
- EXPECT_EQ(owned.get(), inner.Get());
- }
-}
-
TEST(UnownedPtr, ReleaseNotOk) {
#if defined(ADDRESS_SANITIZER)
EXPECT_DEATH(ReleaseDangling(), "");
