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
This commit is contained in:
bungeman 2016-03-08 08:35:23 -08:00 committed by Commit bot
parent ece8392438
commit c901c11549
3 changed files with 161 additions and 17 deletions

View File

@ -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)...));

View File

@ -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 {

View File

@ -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 {