From b4eccc82278472a993b06f8bce19806752702499 Mon Sep 17 00:00:00 2001 From: Rafael Cintron Date: Wed, 22 Apr 2020 20:34:00 +0000 Subject: [PATCH] Allow Ref -> Ref assignment, and move operations Previous to this change, you were unable to assign or move Ref to a Ref. This change addresses the problem by introducing versions of assignment, copy and move methods. nullptr_t specific ones were also added to disambiguate things for the compiler. Bug:dawn:390 Change-Id: Ib5d44231e26db35de33d63c67b36b5bf411a3540 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/20121 Commit-Queue: Rafael Cintron Reviewed-by: Corentin Wallez --- src/common/RefCounted.h | 52 +++++++- src/tests/unittests/RefCountedTests.cpp | 165 +++++++++++++++++++++++- 2 files changed, 208 insertions(+), 9 deletions(-) diff --git a/src/common/RefCounted.h b/src/common/RefCounted.h index 05cef94784..b055d7b71e 100644 --- a/src/common/RefCounted.h +++ b/src/common/RefCounted.h @@ -41,16 +41,32 @@ class RefCounted { template class Ref { public: - Ref() { + Ref() = default; + + constexpr Ref(std::nullptr_t) { } - Ref(T* p) : mPointee(p) { + Ref& operator=(std::nullptr_t) { + Release(); + mPointee = nullptr; + return *this; + } + + template + Ref(U* p) : mPointee(p) { + static_assert(std::is_convertible::value, ""); Reference(); } Ref(const Ref& other) : mPointee(other.mPointee) { Reference(); } + template + Ref(const Ref& other) : mPointee(other.mPointee) { + static_assert(std::is_convertible::value, ""); + Reference(); + } + Ref& operator=(const Ref& other) { if (&other == this) return *this; @@ -62,10 +78,24 @@ class Ref { return *this; } - Ref(Ref&& other) { + template + Ref& operator=(const Ref& other) { + static_assert(std::is_convertible::value, ""); + + other.Reference(); + Release(); + mPointee = other.mPointee; + + return *this; + } + + template + Ref(Ref&& other) { + static_assert(std::is_convertible::value, ""); mPointee = other.mPointee; other.mPointee = nullptr; } + Ref& operator=(Ref&& other) { if (&other == this) return *this; @@ -76,6 +106,16 @@ class Ref { return *this; } + template + Ref& operator=(Ref&& other) { + static_assert(std::is_convertible::value, ""); + + Release(); + mPointee = other.mPointee; + other.mPointee = nullptr; + + return *this; + } ~Ref() { Release(); @@ -114,6 +154,11 @@ class Ref { } private: + // Friend is needed so that instances of Ref can assign mPointee + // members of Ref. + template + friend class Ref; + void Reference() const { if (mPointee != nullptr) { mPointee->Reference(); @@ -125,7 +170,6 @@ class Ref { } } - // static_assert(std::is_base_of::value, ""); T* mPointee = nullptr; }; diff --git a/src/tests/unittests/RefCountedTests.cpp b/src/tests/unittests/RefCountedTests.cpp index a7ddacff72..5981dc936b 100644 --- a/src/tests/unittests/RefCountedTests.cpp +++ b/src/tests/unittests/RefCountedTests.cpp @@ -17,19 +17,20 @@ #include "common/RefCounted.h" -struct RCTest : public RefCounted { +class RCTest : public RefCounted { + public: RCTest() : RefCounted() { } RCTest(uint64_t payload) : RefCounted(payload) { } - RCTest(bool* deleted) : deleted(deleted) { + RCTest(bool* deleted) : mDeleted(deleted) { } ~RCTest() override { - if (deleted != nullptr) { - *deleted = true; + if (mDeleted != nullptr) { + *mDeleted = true; } } @@ -37,7 +38,19 @@ struct RCTest : public RefCounted { return this; } - bool* deleted = nullptr; + private: + bool* mDeleted = nullptr; +}; + +struct RCTestDerived : public RCTest { + RCTestDerived() : RCTest() { + } + + RCTestDerived(uint64_t payload) : RCTest(payload) { + } + + RCTestDerived(bool* deleted) : RCTest(deleted) { + } }; // Test that RCs start with one ref, and removing it destroys the object. @@ -142,14 +155,21 @@ TEST(Ref, CopyConstructor) { RCTest* original = new RCTest(&deleted); Ref source(original); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + Ref destination(source); + EXPECT_EQ(original->GetRefCountForTesting(), 3u); + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); EXPECT_EQ(source.Get(), original); EXPECT_EQ(destination.Get(), original); source = nullptr; EXPECT_FALSE(deleted); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + destination = nullptr; EXPECT_TRUE(deleted); } @@ -182,8 +202,13 @@ TEST(Ref, MoveConstructor) { RCTest* original = new RCTest(&deleted); Ref source(original); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + Ref destination(std::move(source)); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); EXPECT_EQ(source.Get(), nullptr); EXPECT_EQ(destination.Get(), original); @@ -199,10 +224,14 @@ TEST(Ref, MoveAssignment) { RCTest* original = new RCTest(&deleted); Ref source(original); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); Ref destination; destination = std::move(source); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); EXPECT_EQ(source.Get(), nullptr); EXPECT_EQ(destination.Get(), original); @@ -212,6 +241,31 @@ TEST(Ref, MoveAssignment) { EXPECT_TRUE(deleted); } +// Test move assigment where the destination and source +// point to the same underlying object. +TEST(Ref, MoveAssignmentSameObject) { + bool deleted = false; + RCTest* original = new RCTest(&deleted); + + Ref source(original); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + + Ref& referenceToSource = source; + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + + referenceToSource = std::move(source); + + EXPECT_EQ(source.Get(), original); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + EXPECT_FALSE(deleted); + + source = nullptr; + EXPECT_TRUE(deleted); +} + // Test the payload initial value is set correctly TEST(Ref, InitialPayloadValue) { RCTest* testDefaultConstructor = new RCTest(); @@ -255,4 +309,105 @@ TEST(Ref, Detach) { detached->Release(); EXPECT_TRUE(deleted); +} + +// Test constructor passed a derived pointer +TEST(Ref, DerivedPointerConstructor) { + bool deleted = false; + { + Ref test(new RCTestDerived(&deleted)); + test->Release(); + } + EXPECT_TRUE(deleted); +} + +// Test copy constructor of derived class +TEST(Ref, DerivedCopyConstructor) { + bool deleted = false; + Ref testDerived(new RCTestDerived(&deleted)); + testDerived->Release(); + + { + Ref testBase(testDerived); + EXPECT_EQ(testBase->GetRefCountForTesting(), 2u); + EXPECT_EQ(testDerived->GetRefCountForTesting(), 2u); + } + + EXPECT_EQ(testDerived->GetRefCountForTesting(), 1u); +} + +// Test Ref constructed with nullptr +TEST(Ref, ConstructedWithNullptr) { + Ref test(nullptr); + EXPECT_EQ(test.Get(), nullptr); +} + +// Test Ref's copy assignment with derived class +TEST(Ref, CopyAssignmentDerived) { + bool deleted = false; + + RCTestDerived* original = new RCTestDerived(&deleted); + Ref source(original); + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + + Ref destination; + destination = source; + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + + EXPECT_EQ(source.Get(), original); + EXPECT_EQ(destination.Get(), original); + + source = nullptr; + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + EXPECT_FALSE(deleted); + + destination = nullptr; + EXPECT_TRUE(deleted); +} + +// Test Ref's move constructor with derived class +TEST(Ref, MoveConstructorDerived) { + bool deleted = false; + RCTestDerived* original = new RCTestDerived(&deleted); + + Ref source(original); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + + Ref destination(std::move(source)); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + + EXPECT_EQ(source.Get(), nullptr); + EXPECT_EQ(destination.Get(), original); + EXPECT_FALSE(deleted); + + destination = nullptr; + EXPECT_TRUE(deleted); +} + +// Test Ref's move assignment with derived class +TEST(Ref, MoveAssignmentDerived) { + bool deleted = false; + RCTestDerived* original = new RCTestDerived(&deleted); + + Ref source(original); + EXPECT_EQ(original->GetRefCountForTesting(), 2u); + + original->Release(); + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + + Ref destination; + destination = std::move(source); + + EXPECT_EQ(original->GetRefCountForTesting(), 1u); + + EXPECT_EQ(source.Get(), nullptr); + EXPECT_EQ(destination.Get(), original); + EXPECT_FALSE(deleted); + + destination = nullptr; + EXPECT_TRUE(deleted); } \ No newline at end of file