Merge "Revert "Fix wp and sp comparison bugs""
diff --git a/libutils/RefBase_test.cpp b/libutils/RefBase_test.cpp
index b594ca5..2e0cf6e 100644
--- a/libutils/RefBase_test.cpp
+++ b/libutils/RefBase_test.cpp
@@ -45,44 +45,6 @@
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 @@
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<Foo> sp1(foo);
- sp<Foo> sp2(foo2);
- wp<Foo> wp1(sp1);
- wp<Foo> wp2(sp1);
- wp<Foo> 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;
- wp<Foo>wp_smaller = sp1_smaller ? wp1 : wp3;
- wp<Foo>wp_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<FooFixedAlloc> sp1(foo);
- wp<FooFixedAlloc> 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<FooFixedAlloc> sp2(foo2);
- wp<FooFixedAlloc> 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 730d631..1780cf2 100644
--- a/libutils/include/utils/RefBase.h
+++ b/libutils/include/utils/RefBase.h
@@ -171,8 +171,6 @@
#define ANDROID_REF_BASE_H
#include <atomic>
-#include <functional>
-#include <type_traits> // for common_type.
#include <stdint.h>
#include <sys/types.h>
@@ -194,26 +192,19 @@
// ---------------------------------------------------------------------------
#define COMPARE_WEAK(_op_) \
+inline bool operator _op_ (const sp<T>& o) const { \
+ return m_ptr _op_ o.m_ptr; \
+} \
+inline bool operator _op_ (const T* o) const { \
+ return m_ptr _op_ o; \
+} \
+template<typename U> \
+inline bool operator _op_ (const sp<U>& o) const { \
+ return m_ptr _op_ o.m_ptr; \
+} \
template<typename U> \
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<template<typename C> class comparator, typename T, typename U>
-static inline bool _wp_compare_(T* a, U* b) {
- return comparator<typename std::common_type<T*, U*>::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<typename U> \
-inline bool operator _op_ (const U* o) const { \
- return _wp_compare_<_compare_>(m_ptr, o); \
}
// ---------------------------------------------------------------------------
@@ -404,51 +395,39 @@
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<T>& o) const {
+ return (m_ptr == o.m_ptr) && (m_refs == o.m_refs);
+ }
template<typename U>
inline bool operator == (const wp<U>& o) const {
- return m_refs == o.m_refs; // Implies m_ptr == o.mptr; see invariants below.
+ return m_ptr == o.m_ptr;
}
- template<typename U>
- inline bool operator == (const sp<U>& 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<T>& o) const {
+ return (m_ptr == o.m_ptr) ? (m_refs > o.m_refs) : (m_ptr > o.m_ptr);
}
-
- template<typename U>
- inline bool operator != (const sp<U>& o) const {
- return !(*this == o);
- }
-
template<typename U>
inline bool operator > (const wp<U>& o) const {
- if (m_ptr == o.m_ptr) {
- return _wp_compare_<std::greater>(m_refs, o.m_refs);
- } else {
- return _wp_compare_<std::greater>(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<T>& o) const {
+ return (m_ptr == o.m_ptr) ? (m_refs < o.m_refs) : (m_ptr < o.m_ptr);
+ }
template<typename U>
inline bool operator < (const wp<U>& o) const {
- if (m_ptr == o.m_ptr) {
- return _wp_compare_<std::less>(m_refs, o.m_refs);
- } else {
- return _wp_compare_<std::less>(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<T>& o) const { return m_refs != o.m_refs; }
template<typename U> inline bool operator != (const wp<U>& o) const { return !operator == (o); }
+ inline bool operator <= (const wp<T>& o) const { return !operator > (o); }
template<typename U> inline bool operator <= (const wp<U>& o) const { return !operator > (o); }
+ inline bool operator >= (const wp<T>& o) const { return !operator < (o); }
template<typename U> inline bool operator >= (const wp<U>& o) const { return !operator < (o); }
private:
@@ -467,22 +446,6 @@
// ---------------------------------------------------------------------------
// 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<typename T>
wp<T>::wp(T* other)
: m_ptr(other)
@@ -632,7 +595,6 @@
{
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 9cd7c75..1571129 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 <functional>
-#include <type_traits> // for common_type.
-
// ---------------------------------------------------------------------------
namespace android {
@@ -27,12 +24,13 @@
// ---------------------------------------------------------------------------
-// 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<T>& o) const { \
+ return m_ptr _op_ o.m_ptr; \
+} \
+inline bool operator _op_ (const T* o) const { \
+ return m_ptr _op_ o; \
+} \
template<typename U> \
inline bool operator _op_ (const sp<U>& o) const { \
return m_ptr _op_ o.m_ptr; \
@@ -41,27 +39,14 @@
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<T>& o) const { \
+ return m_ptr _op_ o.m_ptr; \
+} \
+template<typename U> \
+inline bool operator _op_ (const wp<U>& o) const { \
+ return m_ptr _op_ o.m_ptr; \
}
-template<template<typename C> class comparator, typename T, typename U>
-static inline bool _sp_compare_(T* a, U* b) {
- return comparator<typename std::common_type<T*, U*>::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<typename U> \
-inline bool operator _op_ (const sp<U>& o) const { \
- return _sp_compare_<_compare_>(m_ptr, o.m_ptr); \
-} \
-template<typename U> \
-inline bool operator _op_ (const U* o) const { \
- return _sp_compare_<_compare_>(m_ptr, o); \
-}
// ---------------------------------------------------------------------------
template<typename T>
@@ -104,23 +89,12 @@
// 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<typename U>
- inline bool operator == (const wp<U>& o) const {
- return o == *this;
- }
-
- template<typename U>
- inline bool operator != (const wp<U>& o) const {
- return o != *this;
- }
+ COMPARE(==)
+ COMPARE(!=)
+ COMPARE(>)
+ COMPARE(<)
+ COMPARE(<=)
+ COMPARE(>=)
private:
template<typename Y> friend class sp;
