Add element_type, swap, operators, fix reset on sk_sp.

The 'element_type' typedef is to play nice with std::pointer_traits.

The full complement of operators and swap to match unique_ptr so that
sk_sp can be properly compared to nullptr and used with standard
containers.

Update to 'reset' so that calling 'unref' is the last operation.

This also adds tests for these changes, and sets the fPtr to nullptr
in debug for easier bug finding.

Review URL: https://codereview.chromium.org/1773453002

diff --git a/include/core/SkRefCnt.h b/include/core/SkRefCnt.h
index d33117751e0717943ec28b890bd645e98ff8f6bc..c3f724fa8ded11926e8f94e16d9e6e8e174d8cae 100644 (file)
--- a/include/core/SkRefCnt.h
+++ b/include/core/SkRefCnt.h
@@ -11,6 +11,7 @@
 #include "../private/SkAtomics.h"
 #include "../private/SkUniquePtr.h"
 #include "SkTypes.h"
+#include <functional>
 #include <utility>
 
 /** \class SkRefCntBase
@@ -241,6 +242,8 @@ template <typename T> class sk_sp {
     /** Supports safe bool idiom. Obsolete with explicit operator bool. */
     using unspecified_bool_type = T* sk_sp::*;
 public:
+    using element_type = T;
+
     sk_sp() : fPtr(nullptr) {}
     sk_sp(std::nullptr_t) : fPtr(nullptr) {}
 
@@ -249,8 +252,7 @@ public:
      *  created sk_sp both have a reference to it.
      */
     sk_sp(const sk_sp<T>& that) : fPtr(SkSafeRef(that.get())) {}
-    template <typename U,
-              typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
+    template <typename U, typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
     sk_sp(const sk_sp<U>& that) : fPtr(SkSafeRef(that.get())) {}
 
     /**
@@ -259,8 +261,7 @@ public:
      *  No call to ref() or unref() will be made.
      */
     sk_sp(sk_sp<T>&& that) : fPtr(that.release()) {}
-    template <typename U,
-              typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
+    template <typename U, typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
     sk_sp(sk_sp<U>&& that) : fPtr(that.release()) {}
 
     /**
@@ -274,6 +275,7 @@ public:
      */
     ~sk_sp() {
         SkSafeUnref(fPtr);
+        SkDEBUGCODE(fPtr = nullptr);
     }
 
     sk_sp<T>& operator=(std::nullptr_t) { this->reset(); return *this; }
@@ -287,8 +289,7 @@ public:
         this->reset(SkSafeRef(that.get()));
         return *this;
     }
-    template <typename U,
-              typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
+    template <typename U, typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
     sk_sp<T>& operator=(const sk_sp<U>& that) {
         this->reset(SkSafeRef(that.get()));
         return *this;
@@ -303,21 +304,12 @@ public:
         this->reset(that.release());
         return *this;
     }
-    template <typename U,
-              typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
+    template <typename U, typename = skstd::enable_if_t<skstd::is_convertible<U*, T*>::value>>
     sk_sp<T>& operator=(sk_sp<U>&& that) {
         this->reset(that.release());
         return *this;
     }
 
-    bool operator==(std::nullptr_t) const { return this->get() == nullptr; }
-    bool operator!=(std::nullptr_t) const { return this->get() != nullptr; }
-
-    template <typename U>
-    bool operator==(const sk_sp<U>& that) const { return this->get() == that.get(); }
-    template <typename U>
-    bool operator!=(const sk_sp<U>& that) const { return this->get() != that.get(); }
-
     T& operator*() const {
         SkASSERT(this->get() != nullptr);
         return *this->get();
@@ -338,8 +330,12 @@ public:
      *  No call to ref() will be made.
      */
     void reset(T* ptr = nullptr) {
-        SkSafeUnref(fPtr);
+        // Calling fPtr->unref() may call this->~() or this->reset(T*).
+        // http://wg21.cmeerw.net/lwg/issue998
+        // http://wg21.cmeerw.net/lwg/issue2262
+        T* oldPtr = fPtr;
         fPtr = ptr;
+        SkSafeUnref(oldPtr);
     }
 
     /**
@@ -353,10 +349,82 @@ public:
         return ptr;
     }
 
+    void swap(sk_sp<T>& that) /*noexcept*/ {
+        using std::swap;
+        swap(fPtr, that.fPtr);
+    }
+
 private:
     T*  fPtr;
 };
 
+template <typename T> inline void swap(sk_sp<T>& a, sk_sp<T>& b) /*noexcept*/ {
+    a.swap(b);
+}
+
+template <typename T, typename U> inline bool operator==(const sk_sp<T>& a, const sk_sp<U>& b) {
+    return a.get() == b.get();
+}
+template <typename T> inline bool operator==(const sk_sp<T>& a, std::nullptr_t) /*noexcept*/ {
+    return !a;
+}
+template <typename T> inline bool operator==(std::nullptr_t, const sk_sp<T>& b) /*noexcept*/ {
+    return !b;
+}
+
+template <typename T, typename U> inline bool operator!=(const sk_sp<T>& a, const sk_sp<U>& b) {
+    return a.get() != b.get();
+}
+template <typename T> inline bool operator!=(const sk_sp<T>& a, std::nullptr_t) /*noexcept*/ {
+    return static_cast<bool>(a);
+}
+template <typename T> inline bool operator!=(std::nullptr_t, const sk_sp<T>& b) /*noexcept*/ {
+    return static_cast<bool>(b);
+}
+
+template <typename T, typename U> inline bool operator<(const sk_sp<T>& a, const sk_sp<U>& b) {
+    // Provide defined total order on sk_sp.
+    // http://wg21.cmeerw.net/lwg/issue1297
+    // http://wg21.cmeerw.net/lwg/issue1401 .
+    return std::less<skstd::common_type_t<T*, U*>>()(a.get(), b.get());
+}
+template <typename T> inline bool operator<(const sk_sp<T>& a, std::nullptr_t) {
+    return std::less<T*>()(a.get(), nullptr);
+}
+template <typename T> inline bool operator<(std::nullptr_t, const sk_sp<T>& b) {
+    return std::less<T*>()(nullptr, b.get());
+}
+
+template <typename T, typename U> inline bool operator<=(const sk_sp<T>& a, const sk_sp<U>& b) {
+    return !(b < a);
+}
+template <typename T> inline bool operator<=(const sk_sp<T>& a, std::nullptr_t) {
+    return !(nullptr < a);
+}
+template <typename T> inline bool operator<=(std::nullptr_t, const sk_sp<T>& b) {
+    return !(b < nullptr);
+}
+
+template <typename T, typename U> inline bool operator>(const sk_sp<T>& a, const sk_sp<U>& b) {
+    return b < a;
+}
+template <typename T> inline bool operator>(const sk_sp<T>& a, std::nullptr_t) {
+    return nullptr < a;
+}
+template <typename T> inline bool operator>(std::nullptr_t, const sk_sp<T>& b) {
+    return b < nullptr;
+}
+
+template <typename T, typename U> inline bool operator>=(const sk_sp<T>& a, const sk_sp<U>& b) {
+    return !(a < b);
+}
+template <typename T> inline bool operator>=(const sk_sp<T>& a, std::nullptr_t) {
+    return !(a < nullptr);
+}
+template <typename T> inline bool operator>=(std::nullptr_t, const sk_sp<T>& b) {
+    return !(nullptr < b);
+}
+
 template <typename T, typename... Args>
 sk_sp<T> sk_make_sp(Args&&... args) {
     return sk_sp<T>(new T(std::forward<Args>(args)...));

diff --git a/include/private/SkTLogic.h b/include/private/SkTLogic.h
index a11953705cf1abcb7e4926c502c6e0065b9ff16d..b38fd50435cbccfd5de0dbeff15eca9e0d9fdd88 100644 (file)
--- a/include/private/SkTLogic.h
+++ b/include/private/SkTLogic.h
@@ -62,6 +62,8 @@ template <typename T> using add_cv_t = typename std::add_cv<T>::type;
 template <typename T> using add_pointer_t = typename std::add_pointer<T>::type;
 template <typename T> using add_lvalue_reference_t = typename std::add_lvalue_reference<T>::type;
 
+template <typename... T> using common_type_t = typename std::common_type<T...>::type;
+
 template <typename S, typename D,
           bool=std::is_void<S>::value || is_function<D>::value || std::is_array<D>::value>
 struct is_convertible_detector {

diff --git a/tests/RefCntTest.cpp b/tests/RefCntTest.cpp
index d3cda7f38d38b2a52c08a7ed97ce7808444d4618..2932913c3da418ac60d5f5aa5ffddcd90bf2de2d 100644 (file)
--- a/tests/RefCntTest.cpp
+++ b/tests/RefCntTest.cpp
@@ -275,6 +275,80 @@ DEF_TEST(sk_sp, reporter) {
     check(reporter, 1, 1, 1, 0);
     paint.set(nullptr);
     check(reporter, 1, 2, 1, 1);
+
+    {
+        sk_sp<SkRefCnt> empty;
+        sk_sp<SkRefCnt> notEmpty = sk_make_sp<SkRefCnt>();
+        REPORTER_ASSERT(reporter, empty == sk_sp<SkRefCnt>());
+
+        REPORTER_ASSERT(reporter, notEmpty != empty);
+        REPORTER_ASSERT(reporter, empty != notEmpty);
+
+        REPORTER_ASSERT(reporter, nullptr == empty);
+        REPORTER_ASSERT(reporter, empty == nullptr);
+        REPORTER_ASSERT(reporter, empty == empty);
+
+        REPORTER_ASSERT(reporter, nullptr <= empty);
+        REPORTER_ASSERT(reporter, empty <= nullptr);
+        REPORTER_ASSERT(reporter, empty <= empty);
+
+        REPORTER_ASSERT(reporter, nullptr >= empty);
+        REPORTER_ASSERT(reporter, empty >= nullptr);
+        REPORTER_ASSERT(reporter, empty >= empty);
+    }
+
+    {
+        sk_sp<SkRefCnt> a = sk_make_sp<SkRefCnt>();
+        sk_sp<SkRefCnt> b = sk_make_sp<SkRefCnt>();
+        REPORTER_ASSERT(reporter, a != b);
+        REPORTER_ASSERT(reporter, (a < b) != (b < a));
+        REPORTER_ASSERT(reporter, (b > a) != (a > b));
+        REPORTER_ASSERT(reporter, (a <= b) != (b <= a));
+        REPORTER_ASSERT(reporter, (b >= a) != (a >= b));
+
+        REPORTER_ASSERT(reporter, a == a);
+        REPORTER_ASSERT(reporter, a <= a);
+        REPORTER_ASSERT(reporter, a >= a);
+    }
+
+    // http://wg21.cmeerw.net/lwg/issue998
+    {
+        class foo : public SkRefCnt {
+        public:
+            foo() : bar(this) {}
+            void reset() { bar.reset(); }
+        private:
+            sk_sp<foo> bar;
+        };
+        // The following should properly delete the object and not cause undefined behavior.
+        // This is an ugly example, but the same issue can arise in more subtle ways.
+        (new foo)->reset();
+    }
+
+    // https://crrev.com/0d4ef2583a6f19c3e61be04d36eb1a60b133832c
+    {
+        struct StructB;
+        struct StructA : public SkRefCnt {
+            sk_sp<StructB> b;
+        };
+
+        struct StructB : public SkRefCnt {
+            sk_sp<StructA> a;
+            ~StructB() override {}; // Some clang versions don't emit this implicitly.
+        };
+
+        // Create a reference cycle.
+        StructA* a = new StructA;
+        a->b.reset(new StructB);
+        a->b->a.reset(a);
+
+        // Break the cycle by calling reset(). This will cause |a| (and hence, |a.b|)
+        // to be deleted before the call to reset() returns. This tests that the
+        // implementation of sk_sp::reset() doesn't access |this| after it
+        // deletes the underlying pointer. This behaviour is consistent with the
+        // definition of unique_ptr::reset in C++11.
+        a->b.reset();
+    }
 }
 
 namespace {