From 91786b7daa901ffda7ef2b0ae8db7cf667b1e12d Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Wed, 10 Nov 2021 08:48:57 +0000 Subject: [PATCH 01/11] Added self assignment tests --- tests/runtime_tests.cpp | 202 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+) diff --git a/tests/runtime_tests.cpp b/tests/runtime_tests.cpp index b1304b6..db2bb31 100644 --- a/tests/runtime_tests.cpp +++ b/tests/runtime_tests.cpp @@ -748,6 +748,102 @@ TEST_CASE("owner move assignment operator valid to valid with deleter", "[owner_ REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("owner move assignment operator self to self", "[owner_assignment]") { + memory_tracker mem_track; + + { + test_ptr ptr(new test_object); + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner move assignment operator self to self sealed", "[owner_assignment]") { + memory_tracker mem_track; + + { + test_sptr ptr = oup::make_observable_sealed(); + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner move assignment operator self to self with deleter", "[owner_assignment]") { + memory_tracker mem_track; + + { + test_ptr_with_deleter ptr(new test_object, test_deleter{42}); + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + REQUIRE(instances_deleter == 1); + REQUIRE(ptr.get_deleter().state_ == 0); + } + + REQUIRE(instances == 0); + REQUIRE(instances_deleter == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner move assignment operator self to self empty", "[owner_assignment]") { + memory_tracker mem_track; + + { + test_ptr ptr; + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner move assignment operator self to self empty sealed", "[owner_assignment]") { + memory_tracker mem_track; + + { + test_sptr ptr; + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner move assignment operator self to self empty with deleter", "[owner_assignment]") { + memory_tracker mem_track; + + { + test_ptr_with_deleter ptr; + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + REQUIRE(instances_deleter == 1); + REQUIRE(ptr.get_deleter().state_ == 0); + } + + REQUIRE(instances == 0); + REQUIRE(instances_deleter == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("owner comparison valid ptr vs nullptr", "[owner_comparison]") { memory_tracker mem_track; @@ -2118,6 +2214,59 @@ TEST_CASE("observer copy assignment operator empty to empty", "[observer_assignm REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("observer copy assignment operator self to self", "[observer_assignment]") { + memory_tracker mem_track; + + { + test_ptr ptr_owner{new test_object}; + test_optr ptr{ptr_owner}; + ptr = ptr; + REQUIRE(instances == 1); + REQUIRE(ptr.get() != nullptr); + REQUIRE(ptr.expired() == false); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer copy assignment operator self to self expired", "[observer_assignment]") { + memory_tracker mem_track; + + { + test_optr ptr; + { + test_ptr ptr_owner{new test_object}; + ptr = ptr_owner; + } + ptr = ptr; + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + REQUIRE(ptr.expired() == true); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer copy assignment operator self to self empty", "[observer_assignment]") { + memory_tracker mem_track; + + { + test_optr ptr; + ptr = ptr; + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + REQUIRE(ptr.expired() == true); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("observer move assignment operator valid to empty", "[observer_assignment]") { memory_tracker mem_track; @@ -2190,6 +2339,59 @@ TEST_CASE("observer move assignment operator empty to empty", "[observer_assignm REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("observer move assignment operator self to self", "[observer_assignment]") { + memory_tracker mem_track; + + { + test_ptr ptr_owner{new test_object}; + test_optr ptr{ptr_owner}; + ptr = std::move(ptr); + REQUIRE(instances == 1); + REQUIRE(ptr.get() == nullptr); + REQUIRE(ptr.expired() == true); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer move assignment operator self to self expired", "[observer_assignment]") { + memory_tracker mem_track; + + { + test_optr ptr; + { + test_ptr ptr_owner{new test_object}; + ptr = ptr_owner; + } + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + REQUIRE(ptr.expired() == true); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer move assignment operator self to self empty", "[observer_assignment]") { + memory_tracker mem_track; + + { + test_optr ptr; + ptr = std::move(ptr); + REQUIRE(instances == 0); + REQUIRE(ptr.get() == nullptr); + REQUIRE(ptr.expired() == true); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("observer acquiring assignment operator valid to empty", "[observer_assignment]") { memory_tracker mem_track; From 56480084060c8f46f7cefe8c4edeef9cca0ad2ec Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Wed, 10 Nov 2021 08:54:17 +0000 Subject: [PATCH 02/11] Fix self-swap and self-assignment --- include/oup/observable_unique_ptr.hpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/oup/observable_unique_ptr.hpp b/include/oup/observable_unique_ptr.hpp index 02cb067..c452649 100644 --- a/include/oup/observable_unique_ptr.hpp +++ b/include/oup/observable_unique_ptr.hpp @@ -292,6 +292,10 @@ class observable_unique_ptr_base { /** \param other The other pointer to swap with */ void swap(observable_unique_ptr_base& other) noexcept { + if (&other == this) { + return; + } + using std::swap; swap(block, other.block); swap(ptr_deleter, other.ptr_deleter); @@ -973,6 +977,10 @@ class observer_ptr { /** \param value The existing weak pointer to copy */ observer_ptr& operator=(const observer_ptr& value) noexcept { + if (&value == this) { + return *this; + } + if (data) { pop_ref_(); } @@ -993,6 +1001,10 @@ class observer_ptr { */ template>> observer_ptr& operator=(const observer_ptr& value) noexcept { + if (&value == this) { + return *this; + } + if (data) { pop_ref_(); } @@ -1114,6 +1126,10 @@ class observer_ptr { /** \param other The other pointer to swap with */ void swap(observer_ptr& other) noexcept { + if (&other == this) { + return; + } + using std::swap; swap(block, other.block); swap(data, other.data); From e7193565cb9da2e7c8d024658240b7b89d5b20ae Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Wed, 10 Nov 2021 08:54:31 +0000 Subject: [PATCH 03/11] Added tests for self swap --- tests/runtime_tests.cpp | 63 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/runtime_tests.cpp b/tests/runtime_tests.cpp index db2bb31..7416754 100644 --- a/tests/runtime_tests.cpp +++ b/tests/runtime_tests.cpp @@ -1387,6 +1387,52 @@ TEST_CASE("owner swap two instances with deleter", "[owner_utility]") { REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("owner swap self", "[owner_utility]") { + memory_tracker mem_track; + + { + test_ptr ptr(new test_object); + ptr.swap(ptr); + REQUIRE(instances == 1); + REQUIRE(ptr.get() != nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner swap self sealed", "[owner_utility]") { + memory_tracker mem_track; + + { + test_sptr ptr = oup::make_observable_sealed(); + ptr.swap(ptr); + REQUIRE(instances == 1); + REQUIRE(ptr.get() != nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("owner swap self with deleter", "[owner_utility]") { + memory_tracker mem_track; + + { + test_ptr_with_deleter ptr(new test_object, test_deleter{43}); + ptr.swap(ptr); + REQUIRE(instances == 1); + REQUIRE(ptr.get() != nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(instances_deleter == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("owner dereference", "[owner_utility]") { memory_tracker mem_track; @@ -2082,6 +2128,23 @@ TEST_CASE("observer swap two different instances", "[observer_utility]") { REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("observer swap self", "[observer_utility]") { + memory_tracker mem_track; + + { + test_ptr ptr_owner(new test_object); + test_optr ptr(ptr_owner); + ptr.swap(ptr); + REQUIRE(instances == 1); + REQUIRE(ptr.get() == ptr_owner.get()); + REQUIRE(ptr.expired() == false); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("observer dereference", "[observer_utility]") { memory_tracker mem_track; From 1bf25e08b3925f33f3dbb95dffcd5bd8f5235264 Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Wed, 10 Nov 2021 08:55:00 +0000 Subject: [PATCH 04/11] Added enable_observer_from_this --- include/oup/observable_unique_ptr.hpp | 86 ++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/include/oup/observable_unique_ptr.hpp b/include/oup/observable_unique_ptr.hpp index c452649..b40b1df 100644 --- a/include/oup/observable_unique_ptr.hpp +++ b/include/oup/observable_unique_ptr.hpp @@ -11,6 +11,9 @@ namespace oup { template class observer_ptr; +template +class enable_observer_from_this; + namespace details { struct control_block { enum flag_elements { @@ -106,13 +109,24 @@ class observable_unique_ptr_base { delete_and_pop_ref_(block, ptr_deleter.data, ptr_deleter); } + /// Fill in the observer pointer for objects inheriting from enable_observer_from_this. + void set_this_observer() noexcept { + if constexpr (std::is_base_of_v, T>) { + if (ptr_deleter.data) { + ptr_deleter.data->this_observer = *this; + } + } + } + /// Private constructor using pre-allocated control block. /** \param ctrl The control block pointer * \param value The pointer to own * \note This is used by make_observable_unique(). */ observable_unique_ptr_base(control_block_type* ctrl, T* value) noexcept : - block(ctrl), ptr_deleter{Deleter{}, value} {} + block(ctrl), ptr_deleter{Deleter{}, value} { + set_this_observer(); + } /// Private constructor using pre-allocated control block. /** \param ctrl The control block pointer @@ -121,7 +135,9 @@ class observable_unique_ptr_base { * \note This is used by make_observable_sealed(). */ observable_unique_ptr_base(control_block_type* ctrl, T* value, Deleter del) noexcept : - block(ctrl), ptr_deleter{std::move(del), value} {} + block(ctrl), ptr_deleter{std::move(del), value} { + set_this_observer(); + } // Friendship is required for conversions. template @@ -181,6 +197,7 @@ class observable_unique_ptr_base { observable_unique_ptr_base(value.block, value.ptr_deleter.data, std::move(value.ptr_deleter)) { value.block = nullptr; value.ptr_deleter.data = nullptr; + set_this_observer(); } /// Transfer ownership by implicit casting @@ -195,6 +212,7 @@ class observable_unique_ptr_base { observable_unique_ptr_base(value.block, value.ptr_deleter.data, std::move(value.ptr_deleter)) { value.block = nullptr; value.ptr_deleter.data = nullptr; + set_this_observer(); } /// Transfer ownership by explicit casting @@ -209,6 +227,7 @@ class observable_unique_ptr_base { observable_unique_ptr_base(manager.block, value) { manager.block = nullptr; manager.ptr_deleter.data = nullptr; + set_this_observer(); } /// Transfer ownership by explicit casting @@ -223,6 +242,7 @@ class observable_unique_ptr_base { observable_unique_ptr_base(manager.block, value, std::move(del)) { manager.block = nullptr; manager.ptr_deleter.data = nullptr; + set_this_observer(); } /// Transfer ownership by implicit casting @@ -240,6 +260,7 @@ class observable_unique_ptr_base { ptr_deleter.data = value.ptr_deleter.data; value.ptr_deleter.data = nullptr; static_cast(ptr_deleter) = std::move(static_cast(value.ptr_deleter)); + set_this_observer(); return *this; } @@ -262,6 +283,7 @@ class observable_unique_ptr_base { ptr_deleter.data = value.ptr_deleter.data; value.ptr_deleter.data = nullptr; static_cast(ptr_deleter) = std::move(static_cast(ptr_deleter)); + set_this_observer(); return *this; } @@ -299,6 +321,8 @@ class observable_unique_ptr_base { using std::swap; swap(block, other.block); swap(ptr_deleter, other.ptr_deleter); + other.set_this_observer(); + set_this_observer(); } /// Replaces the managed object with a null pointer. @@ -1169,6 +1193,64 @@ bool operator!= (const observer_ptr& first, const observer_ptr& second) no return first.get() != second.get(); } +/// Enables creating an observer pointer from 'this'. +/** If an object must be able to create an observer pointer to itself, +* without having direct access to the owner pointer (unique or sealed), +* then the object's class can inherit from enable_observer_from_this. +* This provides the observer_from_this() member function, which returns +* a new observer pointer to the object. For this mechanism to work, +* the class must inherit publicly from enable_observer_from_this, +* and the object must be owned by a unique or sealed pointer when +* calling observer_from_this(). If the latter condition is not satisfied, +* i.e., the object was allocated on the stack, or is owned by another +* type of smart pointer, then observer_from_this() will return nullptr. +*/ +template +class enable_observer_from_this { + observer_ptr this_observer; + + // Friendship is required for assignement of the observer. + template + friend class observable_unique_ptr_base; + +protected: + enable_observer_from_this() noexcept = default; + + enable_observer_from_this(const enable_observer_from_this&) noexcept { + // Do not copy the other object's observer, this would be an + // invalid reference. + }; + + enable_observer_from_this(enable_observer_from_this&&) noexcept { + // Do not move the other object's observer, this would be an + // invalid reference. + }; + + ~enable_observer_from_this() noexcept = default; + +public: + + /// Return an observer pointer to 'this'. + /** \return A new observer pointer pointing to 'this'. + * \note If 'this' is not owned by a unique or sealed pointer, i.e., if + * the object was allocated on the stack, or if it is owned by another + * type of smart pointer, then this function will return nullptr. + */ + observer_ptr observer_from_this() { + return this_observer; + } + + /// Return a const observer pointer to 'this'. + /** \return A new observer pointer pointing to 'this'. + * \note If 'this' is not owned by a unique or sealed pointer, i.e., if + * the object was allocated on the stack, or if it is owned by another + * type of smart pointer, then this function will return nullptr. + */ + observer_ptr observer_from_this() const { + return this_observer; + } +}; + } #endif From 9f24026c24c5fc36fffe2f80117aafb88576cf86 Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 14:54:35 +0000 Subject: [PATCH 05/11] Fixed mismatched delete/delete[] --- include/oup/observable_unique_ptr.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/oup/observable_unique_ptr.hpp b/include/oup/observable_unique_ptr.hpp index b40b1df..e8cfdde 100644 --- a/include/oup/observable_unique_ptr.hpp +++ b/include/oup/observable_unique_ptr.hpp @@ -772,7 +772,7 @@ observable_sealed_ptr make_observable_sealed(Args&& ... args) { // Allocate memory constexpr std::size_t block_size = sizeof(block_type); constexpr std::size_t object_size = sizeof(T); - std::byte* buffer = new std::byte[block_size + object_size]; + std::byte* buffer = reinterpret_cast(operator new(block_size + object_size)); try { // Construct control block and object @@ -784,7 +784,7 @@ observable_sealed_ptr make_observable_sealed(Args&& ... args) { } catch (...) { // Exception thrown during object construction, // clean up memory and let exception propagate - delete[] buffer; + delete buffer; throw; } } From 309563a97cd626b58a2c942f18e44ee122be661a Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 14:54:50 +0000 Subject: [PATCH 06/11] Added tests for enable_observer_from_this and fixed implementation --- include/oup/observable_unique_ptr.hpp | 112 +++++++----------- tests/runtime_tests.cpp | 160 ++++++++++++++++++++++++++ tests/tests_common.hpp | 8 ++ 3 files changed, 211 insertions(+), 69 deletions(-) diff --git a/include/oup/observable_unique_ptr.hpp b/include/oup/observable_unique_ptr.hpp index e8cfdde..c45b347 100644 --- a/include/oup/observable_unique_ptr.hpp +++ b/include/oup/observable_unique_ptr.hpp @@ -110,10 +110,11 @@ class observable_unique_ptr_base { } /// Fill in the observer pointer for objects inheriting from enable_observer_from_this. - void set_this_observer() noexcept { + void set_this_observer_() noexcept { if constexpr (std::is_base_of_v, T>) { if (ptr_deleter.data) { - ptr_deleter.data->this_observer = *this; + ptr_deleter.data->this_observer.set_data_(block, ptr_deleter.data); + ++block->refcount; } } } @@ -124,9 +125,7 @@ class observable_unique_ptr_base { * \note This is used by make_observable_unique(). */ observable_unique_ptr_base(control_block_type* ctrl, T* value) noexcept : - block(ctrl), ptr_deleter{Deleter{}, value} { - set_this_observer(); - } + block(ctrl), ptr_deleter{Deleter{}, value} {} /// Private constructor using pre-allocated control block. /** \param ctrl The control block pointer @@ -135,9 +134,7 @@ class observable_unique_ptr_base { * \note This is used by make_observable_sealed(). */ observable_unique_ptr_base(control_block_type* ctrl, T* value, Deleter del) noexcept : - block(ctrl), ptr_deleter{std::move(del), value} { - set_this_observer(); - } + block(ctrl), ptr_deleter{std::move(del), value} {} // Friendship is required for conversions. template @@ -197,7 +194,6 @@ class observable_unique_ptr_base { observable_unique_ptr_base(value.block, value.ptr_deleter.data, std::move(value.ptr_deleter)) { value.block = nullptr; value.ptr_deleter.data = nullptr; - set_this_observer(); } /// Transfer ownership by implicit casting @@ -212,7 +208,6 @@ class observable_unique_ptr_base { observable_unique_ptr_base(value.block, value.ptr_deleter.data, std::move(value.ptr_deleter)) { value.block = nullptr; value.ptr_deleter.data = nullptr; - set_this_observer(); } /// Transfer ownership by explicit casting @@ -227,7 +222,6 @@ class observable_unique_ptr_base { observable_unique_ptr_base(manager.block, value) { manager.block = nullptr; manager.ptr_deleter.data = nullptr; - set_this_observer(); } /// Transfer ownership by explicit casting @@ -242,7 +236,6 @@ class observable_unique_ptr_base { observable_unique_ptr_base(manager.block, value, std::move(del)) { manager.block = nullptr; manager.ptr_deleter.data = nullptr; - set_this_observer(); } /// Transfer ownership by implicit casting @@ -260,7 +253,6 @@ class observable_unique_ptr_base { ptr_deleter.data = value.ptr_deleter.data; value.ptr_deleter.data = nullptr; static_cast(ptr_deleter) = std::move(static_cast(value.ptr_deleter)); - set_this_observer(); return *this; } @@ -283,7 +275,6 @@ class observable_unique_ptr_base { ptr_deleter.data = value.ptr_deleter.data; value.ptr_deleter.data = nullptr; static_cast(ptr_deleter) = std::move(static_cast(ptr_deleter)); - set_this_observer(); return *this; } @@ -321,8 +312,6 @@ class observable_unique_ptr_base { using std::swap; swap(block, other.block); swap(ptr_deleter, other.ptr_deleter); - other.set_this_observer(); - set_this_observer(); } /// Replaces the managed object with a null pointer. @@ -409,21 +398,6 @@ class observable_unique_ptr : return new control_block_type; } - static void pop_ref_(control_block_type* block) noexcept { - --block->refcount; - if (block->refcount == 0) { - delete block; - } - } - - static void delete_and_pop_ref_(control_block_type* block, T* data, Deleter& deleter) noexcept { - deleter(data); - - block->set_expired(); - - pop_ref_(block); - } - // Friendship is required for conversions. template friend class observer_ptr; @@ -461,7 +435,9 @@ class observable_unique_ptr : * using make_observable_unique() instead of this constructor. */ explicit observable_unique_ptr(T* value) : - base(value != nullptr ? allocate_block_() : nullptr, value) {} + base(value != nullptr ? allocate_block_() : nullptr, value) { + base::set_this_observer_(); + } /// Explicit ownership capture of a raw pointer, with customer deleter. /** \param value The raw pointer to take ownership of @@ -471,7 +447,9 @@ class observable_unique_ptr : * using make_observable_unique() instead of this constructor. */ explicit observable_unique_ptr(T* value, Deleter del) : - base(value != nullptr ? allocate_block_() : nullptr, value, std::move(del)) {} + base(value != nullptr ? allocate_block_() : nullptr, value, std::move(del)) { + base::set_this_observer_(); + } /// Transfer ownership by implicit casting /** \param value The pointer to take ownership from @@ -502,7 +480,9 @@ class observable_unique_ptr : */ template observable_unique_ptr(observable_unique_ptr&& manager, T* value) noexcept : - base(std::move(manager), value) {} + base(std::move(manager), value) { + base::set_this_observer_(); + } /// Transfer ownership by explicit casting /** \param manager The smart pointer to take ownership from @@ -513,7 +493,9 @@ class observable_unique_ptr : */ template observable_unique_ptr(observable_unique_ptr&& manager, T* value, Deleter del) noexcept : - base(std::move(manager), value, del) {} + base(std::move(manager), value, del) { + base::set_this_observer_(); + } /// Transfer ownership by implicit casting /** \param value The pointer to take ownership from @@ -572,8 +554,10 @@ class observable_unique_ptr : // Delete the old pointer // (this follows std::unique_ptr specs) if (old_ptr) { - delete_and_pop_ref_(old_block, old_ptr, base::ptr_deleter); + base::delete_and_pop_ref_(old_block, old_ptr, base::ptr_deleter); } + + base::set_this_observer_(); } /// Releases ownership of the managed object and mark observers as expired. @@ -636,7 +620,9 @@ class observable_sealed_ptr : * \note This is used by make_observable_sealed(). */ observable_sealed_ptr(control_block_type* ctrl, T* value) noexcept : - base(ctrl, value, oup::placement_delete{}) {} + base(ctrl, value, oup::placement_delete{}) { + base::set_this_observer_(); + } // Friendship is required for conversions. template @@ -863,6 +849,9 @@ class observer_ptr { // Friendship is required for conversions. template friend class observer_ptr; + // Friendship is required for enable_observer_from_this. + template + friend class details::observable_unique_ptr_base; using control_block = details::control_block; @@ -876,6 +865,15 @@ class observer_ptr { } } + void set_data_(control_block* b, T* d) noexcept { + if (data) { + pop_ref_(); + } + + block = b; + data = d; + } + public: /// Type of the pointed object using element_type = T; @@ -964,12 +962,8 @@ class observer_ptr { */ template>> observer_ptr& operator=(const observable_unique_ptr& owner) noexcept { - if (data) { - pop_ref_(); - } + set_data_(owner.block, owner.ptr_deleter.data); - block = owner.block; - data = owner.ptr_deleter.data; if (block) { ++block->refcount; } @@ -984,12 +978,8 @@ class observer_ptr { */ template>> observer_ptr& operator=(const observable_sealed_ptr& owner) noexcept { - if (data) { - pop_ref_(); - } + set_data_(owner.block, owner.ptr_deleter.data); - block = owner.block; - data = owner.ptr_deleter.data; if (block) { ++block->refcount; } @@ -1005,12 +995,8 @@ class observer_ptr { return *this; } - if (data) { - pop_ref_(); - } + set_data_(value.block, value.data); - block = value.block; - data = value.data; if (block) { ++block->refcount; } @@ -1029,12 +1015,8 @@ class observer_ptr { return *this; } - if (data) { - pop_ref_(); - } + set_data_(value.block, value.data); - block = value.block; - data = value.data; if (block) { ++block->refcount; } @@ -1048,13 +1030,9 @@ class observer_ptr { * pointer is set to null and looses ownership. */ observer_ptr& operator=(observer_ptr&& value) noexcept { - if (data) { - pop_ref_(); - } + set_data_(value.block, value.data); - block = value.block; value.block = nullptr; - data = value.data; value.data = nullptr; return *this; @@ -1069,13 +1047,9 @@ class observer_ptr { */ template>> observer_ptr& operator=(observer_ptr&& value) noexcept { - if (data) { - pop_ref_(); - } + set_data_(value.block, value.data); - block = value.block; value.block = nullptr; - data = value.data; value.data = nullptr; return *this; @@ -1207,11 +1181,11 @@ bool operator!= (const observer_ptr& first, const observer_ptr& second) no */ template class enable_observer_from_this { - observer_ptr this_observer; + mutable observer_ptr this_observer; // Friendship is required for assignement of the observer. template - friend class observable_unique_ptr_base; + friend class details::observable_unique_ptr_base; protected: enable_observer_from_this() noexcept = default; diff --git a/tests/runtime_tests.cpp b/tests/runtime_tests.cpp index 7416754..e3d2b1b 100644 --- a/tests/runtime_tests.cpp +++ b/tests/runtime_tests.cpp @@ -2916,3 +2916,163 @@ TEST_CASE("pointers in vector", "[system_tests]") { REQUIRE(mem_track.leaks() == 0u); REQUIRE(mem_track.double_del() == 0u); } + +TEST_CASE("observer from this", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_ptr_from_this ptr{new test_object_observer_from_this}; + const test_ptr_from_this& cptr = ptr; + + test_optr_from_this optr_from_this = ptr->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr.get()); + REQUIRE(optr_from_this_const.get() == ptr.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this sealed", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_sptr_from_this ptr = oup::make_observable_sealed(); + const test_sptr_from_this& cptr = ptr; + + test_optr_from_this optr_from_this = ptr->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr.get()); + REQUIRE(optr_from_this_const.get() == ptr.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this after move", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_ptr_from_this ptr1{new test_object_observer_from_this}; + test_ptr_from_this ptr2{std::move(ptr1)}; + const test_ptr_from_this& cptr2 = ptr2; + + test_optr_from_this optr_from_this = ptr2->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr2->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr2.get()); + REQUIRE(optr_from_this_const.get() == ptr2.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this after move sealed", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_sptr_from_this ptr1 = oup::make_observable_sealed(); + test_sptr_from_this ptr2{std::move(ptr1)}; + const test_sptr_from_this& cptr2 = ptr2; + + test_optr_from_this optr_from_this = ptr2->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr2->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr2.get()); + REQUIRE(optr_from_this_const.get() == ptr2.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this after move assignment", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_ptr_from_this ptr1{new test_object_observer_from_this}; + test_ptr_from_this ptr2; + ptr2 = std::move(ptr1); + + const test_ptr_from_this& cptr2 = ptr2; + test_optr_from_this optr_from_this = ptr2->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr2->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr2.get()); + REQUIRE(optr_from_this_const.get() == ptr2.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this after move assignment sealed", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_sptr_from_this ptr1 = oup::make_observable_sealed(); + test_sptr_from_this ptr2; + ptr2 = std::move(ptr1); + const test_sptr_from_this& cptr2 = ptr2; + + test_optr_from_this optr_from_this = ptr2->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr2->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr2.get()); + REQUIRE(optr_from_this_const.get() == ptr2.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this stack", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_object_observer_from_this obj; + const test_object_observer_from_this& cobj = obj; + + test_optr_from_this optr_from_this = obj.observer_from_this(); + test_optr_from_this_const optr_from_this_const = cobj.observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == true); + REQUIRE(optr_from_this_const.expired() == true); + REQUIRE(optr_from_this.get() == nullptr); + REQUIRE(optr_from_this_const.get() == nullptr); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} diff --git a/tests/tests_common.hpp b/tests/tests_common.hpp index 8dbb5f2..0d21bb5 100644 --- a/tests/tests_common.hpp +++ b/tests/tests_common.hpp @@ -38,6 +38,10 @@ struct test_object_thrower { test_object_thrower& operator=(test_object_thrower&&) = delete; }; +struct test_object_observer_from_this : + public test_object, + public oup::enable_observer_from_this {}; + struct test_deleter { int state_ = 0; @@ -73,6 +77,10 @@ using test_ptr_derived_with_deleter = oup::observable_unique_ptr; using test_sptr_thrower = oup::observable_sealed_ptr; using test_ptr_thrower_with_deleter = oup::observable_unique_ptr; +using test_ptr_from_this = oup::observable_unique_ptr; +using test_sptr_from_this = oup::observable_sealed_ptr; using test_optr = oup::observer_ptr; using test_optr_derived = oup::observer_ptr; +using test_optr_from_this = oup::observer_ptr; +using test_optr_from_this_const = oup::observer_ptr; From 7a8078c539cd9ee2a2421431693056acf65005ec Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 14:58:26 +0000 Subject: [PATCH 07/11] Updated readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8c23fe2..7cffedc 100644 --- a/README.md +++ b/README.md @@ -192,4 +192,4 @@ Detail of the benchmarks: ## Alternative implementation -An alternative implementation of an "observable unique pointer" can be found [here](https://www.codeproject.com/articles/1011134/smart-observers-to-use-with-unique-ptr). It does not compile out of the box with gcc unfortunately, but it does contain more features (like creating an observer pointer from a raw `this`) and lacks others (their `make_observable` always performs two allocations). Have a look to check if this better suits your needs. +An alternative implementation of an "observable unique pointer" can be found [here](https://www.codeproject.com/articles/1011134/smart-observers-to-use-with-unique-ptr). It does not compile out of the box with gcc unfortunately and lacks certain features (their `make_observable` always performs two allocations). Have a look to check if this better suits your needs. From df317e77437cb6abc821ad78191ba25396da7417 Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 17:16:06 +0000 Subject: [PATCH 08/11] Added info on using enable_observer_from_this --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 7cffedc..51ebc21 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,8 @@ int main() { } ``` +As with `std::shared_ptr`/`std::weak_ptr`, if you need to obtain an observer pointer to an object when you only have `this` (i.e., from a member function), you can inherit from `oup::enable_observer_from_this` to gain access to the `observer_from_this()` member function. This function will return a valid observer pointer as long as the object is owned by a unique or sealed pointer, and will return `nullptr` in all other cases. + ## Limitations From 150a829da34ffd6f67e553f0e0d274100ce23542 Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 17:16:24 +0000 Subject: [PATCH 09/11] Added checks for observer_from_this after release/reset --- tests/runtime_tests.cpp | 51 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/runtime_tests.cpp b/tests/runtime_tests.cpp index e3d2b1b..27a8b6d 100644 --- a/tests/runtime_tests.cpp +++ b/tests/runtime_tests.cpp @@ -3055,6 +3055,57 @@ TEST_CASE("observer from this after move assignment sealed", "[observer_from_thi REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("observer from this after release", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_ptr_from_this ptr1{new test_object_observer_from_this}; + test_object_observer_from_this* ptr2 = ptr1.release(); + const test_object_observer_from_this* cptr2 = ptr2; + + test_optr_from_this optr_from_this = ptr2->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr2->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == true); + REQUIRE(optr_from_this_const.expired() == true); + REQUIRE(optr_from_this.get() == nullptr); + REQUIRE(optr_from_this_const.get() == nullptr); + + delete ptr2; + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + +TEST_CASE("observer from this after release and reset", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_ptr_from_this ptr1{new test_object_observer_from_this}; + test_object_observer_from_this* ptr2 = ptr1.release(); + const test_object_observer_from_this* cptr2 = ptr2; + + test_ptr_from_this ptr3; + ptr3.reset(ptr2); + + test_optr_from_this optr_from_this = ptr2->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr2->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr2); + REQUIRE(optr_from_this_const.get() == ptr2); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("observer from this stack", "[observer_from_this]") { memory_tracker mem_track; From cd42aba1f6d32c52ffa622edce1b80a4145d70d9 Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 18:59:50 +0000 Subject: [PATCH 10/11] Fixed typo in docs --- include/oup/observable_unique_ptr.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/oup/observable_unique_ptr.hpp b/include/oup/observable_unique_ptr.hpp index c45b347..f97bbd6 100644 --- a/include/oup/observable_unique_ptr.hpp +++ b/include/oup/observable_unique_ptr.hpp @@ -1183,7 +1183,7 @@ template class enable_observer_from_this { mutable observer_ptr this_observer; - // Friendship is required for assignement of the observer. + // Friendship is required for assignment of the observer. template friend class details::observable_unique_ptr_base; From 91ef2e1f4a0683f0c88fd92861a886bceba39bec Mon Sep 17 00:00:00 2001 From: Corentin Schreiber Date: Thu, 11 Nov 2021 19:18:14 +0000 Subject: [PATCH 11/11] Support enable_observer_from_this with derived classes --- include/oup/observable_unique_ptr.hpp | 10 ++++++++-- tests/runtime_tests.cpp | 22 ++++++++++++++++++++++ tests/tests_common.hpp | 7 +++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/include/oup/observable_unique_ptr.hpp b/include/oup/observable_unique_ptr.hpp index f97bbd6..ca66c17 100644 --- a/include/oup/observable_unique_ptr.hpp +++ b/include/oup/observable_unique_ptr.hpp @@ -15,6 +15,7 @@ template class enable_observer_from_this; namespace details { + struct control_block { enum flag_elements { flag_none = 0, @@ -37,6 +38,7 @@ template struct ptr_and_deleter : Deleter { T* data = nullptr; }; + } /// Simple default deleter @@ -78,6 +80,9 @@ struct placement_delete }; namespace details { + +struct enable_observer_from_this_base {}; + template> class observable_unique_ptr_base { protected: @@ -111,7 +116,7 @@ class observable_unique_ptr_base { /// Fill in the observer pointer for objects inheriting from enable_observer_from_this. void set_this_observer_() noexcept { - if constexpr (std::is_base_of_v, T>) { + if constexpr (std::is_base_of_v) { if (ptr_deleter.data) { ptr_deleter.data->this_observer.set_data_(block, ptr_deleter.data); ++block->refcount; @@ -362,6 +367,7 @@ class observable_unique_ptr_base { return ptr_deleter.data != nullptr; } }; + } /// Unique-ownership smart pointer, can be observed by observer_ptr, ownership can be released. @@ -1180,7 +1186,7 @@ bool operator!= (const observer_ptr& first, const observer_ptr& second) no * type of smart pointer, then observer_from_this() will return nullptr. */ template -class enable_observer_from_this { +class enable_observer_from_this : public details::enable_observer_from_this_base { mutable observer_ptr this_observer; // Friendship is required for assignment of the observer. diff --git a/tests/runtime_tests.cpp b/tests/runtime_tests.cpp index 27a8b6d..fd29e73 100644 --- a/tests/runtime_tests.cpp +++ b/tests/runtime_tests.cpp @@ -2961,6 +2961,28 @@ TEST_CASE("observer from this sealed", "[observer_from_this]") { REQUIRE(mem_track.double_del() == 0u); } +TEST_CASE("observer from this derived", "[observer_from_this]") { + memory_tracker mem_track; + + { + test_ptr_from_this_derived ptr{new test_object_observer_from_this_derived}; + const test_ptr_from_this_derived& cptr = ptr; + + test_optr_from_this optr_from_this = ptr->observer_from_this(); + test_optr_from_this_const optr_from_this_const = cptr->observer_from_this(); + + REQUIRE(instances == 1); + REQUIRE(optr_from_this.expired() == false); + REQUIRE(optr_from_this_const.expired() == false); + REQUIRE(optr_from_this.get() == ptr.get()); + REQUIRE(optr_from_this_const.get() == ptr.get()); + } + + REQUIRE(instances == 0); + REQUIRE(mem_track.leaks() == 0u); + REQUIRE(mem_track.double_del() == 0u); +} + TEST_CASE("observer from this after move", "[observer_from_this]") { memory_tracker mem_track; diff --git a/tests/tests_common.hpp b/tests/tests_common.hpp index 0d21bb5..c8de086 100644 --- a/tests/tests_common.hpp +++ b/tests/tests_common.hpp @@ -42,6 +42,9 @@ struct test_object_observer_from_this : public test_object, public oup::enable_observer_from_this {}; +struct test_object_observer_from_this_derived : + public test_object_observer_from_this {}; + struct test_deleter { int state_ = 0; @@ -79,8 +82,12 @@ using test_sptr_thrower = oup::observable_sealed_ptr; using test_ptr_thrower_with_deleter = oup::observable_unique_ptr; using test_ptr_from_this = oup::observable_unique_ptr; using test_sptr_from_this = oup::observable_sealed_ptr; +using test_ptr_from_this_derived = oup::observable_unique_ptr; +using test_sptr_from_this_derived = oup::observable_sealed_ptr; using test_optr = oup::observer_ptr; using test_optr_derived = oup::observer_ptr; using test_optr_from_this = oup::observer_ptr; using test_optr_from_this_const = oup::observer_ptr; +using test_optr_from_this_derived = oup::observer_ptr; +using test_optr_from_this_derived_const = oup::observer_ptr;