Make UnownedPtr and RetainPtr more similar to each other.
The remaining difference is that UnownedPtr<T> has operator=(T*),
which prevents one test from compiling until it is removed (which
will create a lot of code churn).
-- add new assignment operators.
-- add new tests and simplify existing ones.
-- add some "noexcepts" and "defaults"
-- template/untemplate some methods.
-- add MakeUnowned() helper.
Change-Id: I1189fa2df8d9f5f704c2e6532ca4b481c6c922ec
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/98411
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 99d73fd..eda9517 100644
--- a/core/fxcrt/retain_ptr.h
+++ b/core/fxcrt/retain_ptr.h
@@ -27,20 +27,20 @@
template <class T>
class RetainPtr {
public:
- RetainPtr() = default;
+ RetainPtr() noexcept = default;
// Deliberately implicit to allow returning nullptrs.
// NOLINTNEXTLINE(runtime/explicit)
RetainPtr(std::nullptr_t ptr) {}
- explicit RetainPtr(T* pObj) : m_pObj(pObj) {
+ explicit RetainPtr(T* pObj) noexcept : m_pObj(pObj) {
if (m_pObj)
m_pObj->Retain();
}
// Copy-construct a RetainPtr.
// Required in addition to copy conversion constructor below.
- RetainPtr(const RetainPtr& that) : RetainPtr(that.Get()) {}
+ RetainPtr(const RetainPtr& that) noexcept : RetainPtr(that.Get()) {}
// Move-construct a RetainPtr. After construction, |that| will be NULL.
// Required in addition to move conversion constructor below.
@@ -60,6 +60,7 @@
Unleak(that.Leak());
}
+ // Copy-assign a RetainPtr.
RetainPtr& operator=(const RetainPtr& that) {
if (*this != that)
Reset(that.Get());
@@ -72,6 +73,8 @@
return *this;
}
+ ~RetainPtr() = default;
+
template <class U>
RetainPtr<U> As() const {
return RetainPtr<U>(static_cast<U*>(Get()));
@@ -168,7 +171,8 @@
return RetainPtr<T>(new T(std::forward<Args>(args)...));
}
-// Type-deducing wrapper to make a RetainPtr from an ordinary pointer.
+// Type-deducing wrapper to make a RetainPtr from an ordinary pointer,
+// since equivalent constructor is explicit.
template <typename T>
RetainPtr<T> WrapRetain(T* that) {
return RetainPtr<T>(that);
diff --git a/core/fxcrt/retain_ptr_unittest.cpp b/core/fxcrt/retain_ptr_unittest.cpp
index e3ca7e4..2fdd993 100644
--- a/core/fxcrt/retain_ptr_unittest.cpp
+++ b/core/fxcrt/retain_ptr_unittest.cpp
@@ -298,10 +298,8 @@
RetainPtr<PseudoRetainable> ptr1(&obj);
{
RetainPtr<const PseudoRetainable> ptr2;
- EXPECT_EQ(&obj, ptr1.Get());
- EXPECT_FALSE(ptr2.Get());
ptr2 = std::move(ptr1);
- EXPECT_FALSE(ptr1.Get());
+ EXPECT_FALSE(ptr1);
EXPECT_EQ(&obj, ptr2.Get());
EXPECT_EQ(1, obj.retain_count());
EXPECT_EQ(0, obj.release_count());
diff --git a/core/fxcrt/unowned_ptr.h b/core/fxcrt/unowned_ptr.h
index 6eca3c8..f9946f0 100644
--- a/core/fxcrt/unowned_ptr.h
+++ b/core/fxcrt/unowned_ptr.h
@@ -51,21 +51,39 @@
// Deliberately implicit to allow returning nullptrs.
// NOLINTNEXTLINE(runtime/explicit)
- constexpr UnownedPtr(std::nullptr_t ptr) noexcept {}
+ constexpr UnownedPtr(std::nullptr_t ptr) {}
- template <typename U>
- explicit constexpr UnownedPtr(U* pObj) noexcept : m_pObj(pObj) {}
+ explicit constexpr UnownedPtr(T* pObj) noexcept : m_pObj(pObj) {}
- constexpr UnownedPtr(const UnownedPtr& that) noexcept = default;
+ // Copy-construct an UnownedPtr.
+ // Required in addition to copy conversion constructor below.
+ constexpr UnownedPtr(const UnownedPtr& that) noexcept : m_pObj(that.Get()) {}
// Move-construct an UnownedPtr. After construction, |that| will be NULL.
+ // Required in addition to move conversion constructor below.
constexpr UnownedPtr(UnownedPtr&& that) noexcept : m_pObj(that.Release()) {}
+ // Copy conversion constructor.
+ template <class U,
+ typename = typename std::enable_if<
+ std::is_convertible<U*, T*>::value>::type>
+ UnownedPtr(const UnownedPtr<U>& that) : UnownedPtr(that.Get()) {}
+
+ // Move-conversion constructor.
+ template <class U,
+ typename = typename std::enable_if<
+ std::is_convertible<U*, T*>::value>::type>
+ UnownedPtr(UnownedPtr<U>&& that) noexcept {
+ Reset(that.Release());
+ }
+
+ // Assing an UnownedPtr from a raw ptr.
UnownedPtr& operator=(T* that) noexcept {
Reset(that);
return *this;
}
+ // Copy-assign an UnownedPtr.
UnownedPtr& operator=(const UnownedPtr& that) noexcept {
if (*this != that)
Reset(that.Get());
@@ -132,4 +150,15 @@
using fxcrt::UnownedPtr;
+namespace pdfium {
+
+// Type-deducing wrapper to make an UnownedPtr from an ordinary pointer,
+// since equivalent constructor is explicit.
+template <typename T>
+UnownedPtr<T> WrapUnowned(T* that) {
+ return UnownedPtr<T>(that);
+}
+
+} // namespace pdfium
+
#endif // CORE_FXCRT_UNOWNED_PTR_H_
diff --git a/core/fxcrt/unowned_ptr_unittest.cpp b/core/fxcrt/unowned_ptr_unittest.cpp
index 7862e5e..532d9c4 100644
--- a/core/fxcrt/unowned_ptr_unittest.cpp
+++ b/core/fxcrt/unowned_ptr_unittest.cpp
@@ -64,6 +64,11 @@
} // namespace
+TEST(UnownedPtr, Null) {
+ UnownedPtr<Clink> ptr;
+ EXPECT_FALSE(ptr.Get());
+}
+
TEST(UnownedPtr, PtrOk) {
auto ptr1 = std::make_unique<Clink>();
{
@@ -80,6 +85,22 @@
#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>();
{
@@ -130,17 +151,31 @@
outer = owned.get();
UnownedPtr<Clink> inner(std::move(outer));
EXPECT_FALSE(outer.Get());
+ EXPECT_EQ(owned.get(), inner.Get());
}
}
-TEST(UnownedPtr, MoveAssignOk) {
+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.Get());
+ EXPECT_FALSE(outer);
+ EXPECT_EQ(owned.get(), inner.Get());
}
}
