diff --git a/libutils/RefBase_test.cpp b/libutils/RefBase_test.cpp index b594ca56d..2e0cf6e46 100644 --- a/libutils/RefBase_test.cpp +++ b/libutils/RefBase_test.cpp @@ -45,44 +45,6 @@ private: bool* mDeleted; }; -// A version of Foo that ensures that all objects are allocated at the same -// address. No more than one can be allocated at a time. Thread-hostile. -class FooFixedAlloc : public RefBase { -public: - static void* operator new(size_t size) { - if (mAllocCount != 0) { - abort(); - } - mAllocCount = 1; - if (theMemory == nullptr) { - theMemory = malloc(size); - } - return theMemory; - } - - static void operator delete(void *p) { - if (mAllocCount != 1 || p != theMemory) { - abort(); - } - mAllocCount = 0; - } - - FooFixedAlloc(bool* deleted_check) : mDeleted(deleted_check) { - *mDeleted = false; - } - - ~FooFixedAlloc() { - *mDeleted = true; - } -private: - bool* mDeleted; - static int mAllocCount; - static void* theMemory; -}; - -int FooFixedAlloc::mAllocCount(0); -void* FooFixedAlloc::theMemory(nullptr); - TEST(RefBase, StrongMoves) { bool isDeleted; Foo* foo = new Foo(&isDeleted); @@ -128,91 +90,6 @@ TEST(RefBase, WeakCopies) { ASSERT_FALSE(isDeleted) << "Deletion on wp destruction should no longer occur"; } -TEST(RefBase, Comparisons) { - bool isDeleted, isDeleted2; - Foo* foo = new Foo(&isDeleted); - Foo* foo2 = new Foo(&isDeleted2); - sp sp1(foo); - sp sp2(foo2); - wp wp1(sp1); - wp wp2(sp1); - wp wp3(sp2); - ASSERT_TRUE(wp1 == wp2); - ASSERT_TRUE(wp1 == sp1); - ASSERT_TRUE(wp3 == sp2); - ASSERT_TRUE(wp1 != sp2); - ASSERT_TRUE(wp1 <= wp2); - ASSERT_TRUE(wp1 >= wp2); - ASSERT_FALSE(wp1 != wp2); - ASSERT_FALSE(wp1 > wp2); - ASSERT_FALSE(wp1 < wp2); - ASSERT_FALSE(sp1 == sp2); - ASSERT_TRUE(sp1 != sp2); - bool sp1_smaller = sp1 < sp2; - wpwp_smaller = sp1_smaller ? wp1 : wp3; - wpwp_larger = sp1_smaller ? wp3 : wp1; - ASSERT_TRUE(wp_smaller < wp_larger); - ASSERT_TRUE(wp_smaller != wp_larger); - ASSERT_TRUE(wp_smaller <= wp_larger); - ASSERT_FALSE(wp_smaller == wp_larger); - ASSERT_FALSE(wp_smaller > wp_larger); - ASSERT_FALSE(wp_smaller >= wp_larger); - sp2 = nullptr; - ASSERT_TRUE(isDeleted2); - ASSERT_FALSE(isDeleted); - ASSERT_FALSE(wp3 == sp2); - // Comparison results on weak pointers should not be affected. - ASSERT_TRUE(wp_smaller < wp_larger); - ASSERT_TRUE(wp_smaller != wp_larger); - ASSERT_TRUE(wp_smaller <= wp_larger); - ASSERT_FALSE(wp_smaller == wp_larger); - ASSERT_FALSE(wp_smaller > wp_larger); - ASSERT_FALSE(wp_smaller >= wp_larger); - wp2 = nullptr; - ASSERT_FALSE(wp1 == wp2); - ASSERT_TRUE(wp1 != wp2); - wp1.clear(); - ASSERT_TRUE(wp1 == wp2); - ASSERT_FALSE(wp1 != wp2); - wp3.clear(); - ASSERT_TRUE(wp1 == wp3); - ASSERT_FALSE(wp1 != wp3); - ASSERT_FALSE(isDeleted); - sp1.clear(); - ASSERT_TRUE(isDeleted); - ASSERT_TRUE(sp1 == sp2); -} - -// Check whether comparison against dead wp works, even if the object referenced -// by the new wp happens to be at the same address. -TEST(RefBase, ReplacedComparison) { - bool isDeleted, isDeleted2; - FooFixedAlloc* foo = new FooFixedAlloc(&isDeleted); - sp sp1(foo); - wp wp1(sp1); - ASSERT_TRUE(wp1 == sp1); - sp1.clear(); // Deallocates the object. - ASSERT_TRUE(isDeleted); - FooFixedAlloc* foo2 = new FooFixedAlloc(&isDeleted2); - ASSERT_FALSE(isDeleted2); - ASSERT_EQ(foo, foo2); // Not technically a legal comparison, but ... - sp sp2(foo2); - wp wp2(sp2); - ASSERT_TRUE(sp2 == wp2); - ASSERT_FALSE(sp2 != wp2); - ASSERT_TRUE(sp2 != wp1); - ASSERT_FALSE(sp2 == wp1); - ASSERT_FALSE(sp2 == sp1); // sp1 is null. - ASSERT_FALSE(wp1 == wp2); // wp1 refers to old object. - ASSERT_TRUE(wp1 != wp2); - ASSERT_TRUE(wp1 > wp2 || wp1 < wp2); - ASSERT_TRUE(wp1 >= wp2 || wp1 <= wp2); - ASSERT_FALSE(wp1 >= wp2 && wp1 <= wp2); - ASSERT_FALSE(wp1 == nullptr); - wp1 = sp2; - ASSERT_TRUE(wp1 == wp2); - ASSERT_FALSE(wp1 != wp2); -} // Set up a situation in which we race with visit2AndRremove() to delete // 2 strong references. Bar destructor checks that there are no early diff --git a/libutils/include/utils/RefBase.h b/libutils/include/utils/RefBase.h index 730d63153..1780cf22f 100644 --- a/libutils/include/utils/RefBase.h +++ b/libutils/include/utils/RefBase.h @@ -171,8 +171,6 @@ #define ANDROID_REF_BASE_H #include -#include -#include // for common_type. #include #include @@ -194,26 +192,19 @@ TextOutput& printWeakPointer(TextOutput& to, const void* val); // --------------------------------------------------------------------------- #define COMPARE_WEAK(_op_) \ +inline bool operator _op_ (const sp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ +inline bool operator _op_ (const T* o) const { \ + return m_ptr _op_ o; \ +} \ +template \ +inline bool operator _op_ (const sp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ template \ inline bool operator _op_ (const U* o) const { \ return m_ptr _op_ o; \ -} \ -/* Needed to handle type inference for nullptr: */ \ -inline bool operator _op_ (const T* o) const { \ - return m_ptr _op_ o; \ -} - -template class comparator, typename T, typename U> -static inline bool _wp_compare_(T* a, U* b) { - return comparator::type>()(a, b); -} - -// Use std::less and friends to avoid undefined behavior when ordering pointers -// to different objects. -#define COMPARE_WEAK_FUNCTIONAL(_op_, _compare_) \ -template \ -inline bool operator _op_ (const U* o) const { \ - return _wp_compare_<_compare_>(m_ptr, o); \ } // --------------------------------------------------------------------------- @@ -404,51 +395,39 @@ public: COMPARE_WEAK(==) COMPARE_WEAK(!=) - COMPARE_WEAK_FUNCTIONAL(>, std::greater) - COMPARE_WEAK_FUNCTIONAL(<, std::less) - COMPARE_WEAK_FUNCTIONAL(<=, std::less_equal) - COMPARE_WEAK_FUNCTIONAL(>=, std::greater_equal) + COMPARE_WEAK(>) + COMPARE_WEAK(<) + COMPARE_WEAK(<=) + COMPARE_WEAK(>=) + inline bool operator == (const wp& o) const { + return (m_ptr == o.m_ptr) && (m_refs == o.m_refs); + } template inline bool operator == (const wp& o) const { - return m_refs == o.m_refs; // Implies m_ptr == o.mptr; see invariants below. + return m_ptr == o.m_ptr; } - template - inline bool operator == (const sp& o) const { - // Just comparing m_ptr fields is often dangerous, since wp<> may refer to an older - // object at the same address. - if (o == nullptr) { - return m_ptr == nullptr; - } else { - return m_refs == o->getWeakRefs(); // Implies m_ptr == o.mptr. - } + inline bool operator > (const wp& o) const { + return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr); } - - template - inline bool operator != (const sp& o) const { - return !(*this == o); - } - template inline bool operator > (const wp& o) const { - if (m_ptr == o.m_ptr) { - return _wp_compare_(m_refs, o.m_refs); - } else { - return _wp_compare_(m_ptr, o.m_ptr); - } + return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr); } + inline bool operator < (const wp& o) const { + return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr); + } template inline bool operator < (const wp& o) const { - if (m_ptr == o.m_ptr) { - return _wp_compare_(m_refs, o.m_refs); - } else { - return _wp_compare_(m_ptr, o.m_ptr); - } + return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr); } + inline bool operator != (const wp& o) const { return m_refs != o.m_refs; } template inline bool operator != (const wp& o) const { return !operator == (o); } + inline bool operator <= (const wp& o) const { return !operator > (o); } template inline bool operator <= (const wp& o) const { return !operator > (o); } + inline bool operator >= (const wp& o) const { return !operator < (o); } template inline bool operator >= (const wp& o) const { return !operator < (o); } private: @@ -467,22 +446,6 @@ TextOutput& operator<<(TextOutput& to, const wp& val); // --------------------------------------------------------------------------- // No user serviceable parts below here. -// Implementation invariants: -// Either -// 1) m_ptr and m_refs are both null, or -// 2) m_refs == m_ptr->mRefs, or -// 3) *m_ptr is no longer live, and m_refs points to the weakref_type object that corresponded -// to m_ptr while it was live. *m_refs remains live while a wp<> refers to it. -// -// The m_refs field in a RefBase object is allocated on construction, unique to that RefBase -// object, and never changes. Thus if two wp's have identical m_refs fields, they are either both -// null or point to the same object. If two wp's have identical m_ptr fields, they either both -// point to the same live object and thus have the same m_ref fields, or at least one of the -// objects is no longer live. -// -// Note that the above comparison operations go out of their way to provide an ordering consistent -// with ordinary pointer comparison; otherwise they could ignore m_ptr, and just compare m_refs. - template wp::wp(T* other) : m_ptr(other) @@ -632,7 +595,6 @@ void wp::clear() { if (m_ptr) { m_refs->decWeak(this); - m_refs = 0; m_ptr = 0; } } diff --git a/libutils/include/utils/StrongPointer.h b/libutils/include/utils/StrongPointer.h index 9cd7c75fd..15711296d 100644 --- a/libutils/include/utils/StrongPointer.h +++ b/libutils/include/utils/StrongPointer.h @@ -17,9 +17,6 @@ #ifndef ANDROID_STRONG_POINTER_H #define ANDROID_STRONG_POINTER_H -#include -#include // for common_type. - // --------------------------------------------------------------------------- namespace android { @@ -27,12 +24,13 @@ template class wp; // --------------------------------------------------------------------------- -// TODO: Maybe remove sp<> ? wp<> comparison? These are dangerous: If the wp<> -// was created before the sp<>, and they point to different objects, they may -// compare equal even if they are entirely unrelated. E.g. CameraService -// currently performa such comparisons. - -#define COMPARE_STRONG(_op_) \ +#define COMPARE(_op_) \ +inline bool operator _op_ (const sp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ +inline bool operator _op_ (const T* o) const { \ + return m_ptr _op_ o; \ +} \ template \ inline bool operator _op_ (const sp& o) const { \ return m_ptr _op_ o.m_ptr; \ @@ -41,27 +39,14 @@ template \ inline bool operator _op_ (const U* o) const { \ return m_ptr _op_ o; \ } \ -/* Needed to handle type inference for nullptr: */ \ -inline bool operator _op_ (const T* o) const { \ - return m_ptr _op_ o; \ +inline bool operator _op_ (const wp& o) const { \ + return m_ptr _op_ o.m_ptr; \ +} \ +template \ +inline bool operator _op_ (const wp& o) const { \ + return m_ptr _op_ o.m_ptr; \ } -template class comparator, typename T, typename U> -static inline bool _sp_compare_(T* a, U* b) { - return comparator::type>()(a, b); -} - -// Use std::less and friends to avoid undefined behavior when ordering pointers -// to different objects. -#define COMPARE_STRONG_FUNCTIONAL(_op_, _compare_) \ -template \ -inline bool operator _op_ (const sp& o) const { \ - return _sp_compare_<_compare_>(m_ptr, o.m_ptr); \ -} \ -template \ -inline bool operator _op_ (const U* o) const { \ - return _sp_compare_<_compare_>(m_ptr, o); \ -} // --------------------------------------------------------------------------- template @@ -104,23 +89,12 @@ public: // Operators - COMPARE_STRONG(==) - COMPARE_STRONG(!=) - COMPARE_STRONG_FUNCTIONAL(>, std::greater) - COMPARE_STRONG_FUNCTIONAL(<, std::less) - COMPARE_STRONG_FUNCTIONAL(<=, std::less_equal) - COMPARE_STRONG_FUNCTIONAL(>=, std::greater_equal) - - // Punt these to the wp<> implementation. - template - inline bool operator == (const wp& o) const { - return o == *this; - } - - template - inline bool operator != (const wp& o) const { - return o != *this; - } + COMPARE(==) + COMPARE(!=) + COMPARE(>) + COMPARE(<) + COMPARE(<=) + COMPARE(>=) private: template friend class sp;