From e4684944cbb702877f8aa7c2e248171a6b113a5c Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Fri, 31 May 2024 20:51:09 +0300 Subject: [PATCH] removal of Pmr from InterfacePtr Deleter #verification #docs #sonar --- .../example_07_polymorphic_alloc_deleter.cpp | 8 +- .../unittest/pmr/test_pmr_interface_ptr.cpp | 65 +++++++- .../unittest/test_unbounded_variant.cpp | 1 - include/cetl/pmr/interface_ptr.hpp | 139 +++++++++++++----- 4 files changed, 166 insertions(+), 47 deletions(-) diff --git a/cetlvast/suites/docs/examples/example_07_polymorphic_alloc_deleter.cpp b/cetlvast/suites/docs/examples/example_07_polymorphic_alloc_deleter.cpp index f73ebc1..6a9bdfe 100644 --- a/cetlvast/suites/docs/examples/example_07_polymorphic_alloc_deleter.cpp +++ b/cetlvast/suites/docs/examples/example_07_polymorphic_alloc_deleter.cpp @@ -134,7 +134,7 @@ class example_07_polymorphic_alloc_deleter : public testing::Test { protected: template - using InterfacePtr = cetl::pmr::InterfacePtr; + using InterfacePtr = cetl::pmr::InterfacePtr; void SetUp() override { @@ -253,8 +253,10 @@ TEST_F(example_07_polymorphic_alloc_deleter, example_usage_2) std::cout << "Obj2 desc : " << obj2->describe() << std::endl; std::cout << "Obj2 name_a : " << obj2->name() << std::endl; - auto obj2_named = InterfacePtr{std::move(obj2)}; - std::cout << "Obj2 name_b : " << obj2_named->name() << std::endl; + // Such interface ptr upcasting currently is not supported. + // + // auto obj2_named = InterfacePtr{std::move(obj2)}; + // std::cout << "Obj2 name_b : " << obj2_named->name() << std::endl; } auto obj3 = cetl::pmr::InterfaceFactory::make_unique(alloc, "obj3", 4U); diff --git a/cetlvast/suites/unittest/pmr/test_pmr_interface_ptr.cpp b/cetlvast/suites/unittest/pmr/test_pmr_interface_ptr.cpp index ca1d13b..a491dff 100644 --- a/cetlvast/suites/unittest/pmr/test_pmr_interface_ptr.cpp +++ b/cetlvast/suites/unittest/pmr/test_pmr_interface_ptr.cpp @@ -7,6 +7,7 @@ /// SPDX-License-Identifier: MIT #include +#include #include #include @@ -16,9 +17,20 @@ namespace { +using testing::_; using testing::IsNull; +using testing::Return; using testing::IsEmpty; using testing::NotNull; +using testing::StrictMock; + +#if defined(__cpp_exceptions) + +// Workaround for GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 +// Should be used in the tests where exceptions are expected (see `EXPECT_THROW`). +const auto sink = [](auto&&) {}; + +#endif class INamed { @@ -83,9 +95,15 @@ std::uint32_t MyObjectBase::counter_ = 0; class MyObject final : private MyObjectBase, public IIdentifiable, public IDescribable { public: - MyObject(std::string name) + MyObject(std::string name, bool throw_on_ctor = false) : name_{std::move(name)} { + if (throw_on_ctor) + { +#if defined(__cpp_exceptions) + throw std::runtime_error("ctor"); +#endif + } } ~MyObject() = default; @@ -124,9 +142,6 @@ class TestPmrInterfacePtr : public testing::Test protected: using pmr = cetl::pmr::memory_resource; - template - using InterfacePtr = cetl::pmr::InterfacePtr; - void SetUp() override { MyObjectBase::counter_ = 0; @@ -177,13 +192,47 @@ TEST_F(TestPmrInterfacePtr, up_cast_interface) EXPECT_THAT(obj0->name(), "obj0"); EXPECT_THAT(obj0->describe(), "obj0 is a MyObject instance."); - cetl::pmr::InterfacePtr obj0_named{std::move(obj0)}; + const INamed& obj0_named = *obj0; + + EXPECT_THAT(obj0, NotNull()); + EXPECT_THAT(obj0_named.name(), "obj0"); + + obj0.reset(); +} + +TEST_F(TestPmrInterfacePtr, make_unique_out_of_memory) +{ + StrictMock mr_mock{}; + + cetl::pmr::polymorphic_allocator alloc{&mr_mock}; + EXPECT_CALL(mr_mock, do_allocate(sizeof(MyObject), _)).WillOnce(Return(nullptr)); + + auto obj0 = cetl::pmr::InterfaceFactory::make_unique(alloc, "obj0"); EXPECT_THAT(obj0, IsNull()); - EXPECT_THAT(obj0_named, NotNull()); - EXPECT_THAT(obj0_named->name(), "obj0"); +} - obj0_named.reset(); +TEST_F(TestPmrInterfacePtr, make_unique_myobj_ctor_throws) +{ + StrictMock mr_mock{}; + + cetl::pmr::polymorphic_allocator alloc{&mr_mock}; + + EXPECT_CALL(mr_mock, do_allocate(sizeof(MyObject), _)) + .WillOnce( + [this](std::size_t size_bytes, std::size_t alignment) { return mr_.allocate(size_bytes, alignment); }); + EXPECT_CALL(mr_mock, do_deallocate(_, sizeof(MyObject), _)) + .WillOnce([this](void* p, std::size_t size_bytes, std::size_t alignment) { + mr_.deallocate(p, size_bytes, alignment); + }); + +#if defined(__cpp_exceptions) + EXPECT_THROW(sink(cetl::pmr::InterfaceFactory::make_unique(alloc, "obj0", true)), std::runtime_error); +#else + auto obj0 = cetl::pmr::InterfaceFactory::make_unique(alloc, "obj0", true); + EXPECT_THAT(obj0, NotNull()); + EXPECT_THAT(obj0->name(), "obj0"); +#endif } } // namespace diff --git a/cetlvast/suites/unittest/test_unbounded_variant.cpp b/cetlvast/suites/unittest/test_unbounded_variant.cpp index 6b19f44..f2e4665 100644 --- a/cetlvast/suites/unittest/test_unbounded_variant.cpp +++ b/cetlvast/suites/unittest/test_unbounded_variant.cpp @@ -36,7 +36,6 @@ using testing::Return; using testing::IsNull; using testing::IsEmpty; using testing::NotNull; -using testing::InSequence; using testing::StrictMock; using namespace std::string_literals; diff --git a/include/cetl/pmr/interface_ptr.hpp b/include/cetl/pmr/interface_ptr.hpp index 124852d..020c4f1 100644 --- a/include/cetl/pmr/interface_ptr.hpp +++ b/include/cetl/pmr/interface_ptr.hpp @@ -16,51 +16,57 @@ namespace cetl namespace pmr { -template +template class PmrInterfaceDeleter final { public: template PmrInterfaceDeleter(PmrAllocator alloc, std::size_t obj_count) - : deleter_{alloc.resource(), [alloc, obj_count](Interface* ptr) mutable { - using Concrete = typename PmrAllocator::value_type; - auto* concrete_ptr = static_cast(ptr); + : deleter_{[alloc, obj_count](Interface* ptr) mutable { + using Concrete = typename PmrAllocator::value_type; - concrete_ptr->~Concrete(); - alloc.deallocate(concrete_ptr, obj_count); - }} + auto* concrete_ptr = static_cast(ptr); + concrete_ptr->~Concrete(); + alloc.deallocate(concrete_ptr, obj_count); + }} { } - template ::value>> - PmrInterfaceDeleter(const PmrInterfaceDeleter& other) - : deleter_{other.get_memory_resource(), [other](Interface* ptr) { - // Delegate to the down class deleter. - other.deleter_(static_cast(ptr)); - }} - { - } - - Pmr* get_memory_resource() const noexcept - { - return deleter_.get_memory_resource(); - } - void operator()(Interface* ptr) noexcept { deleter_(ptr); } -private: - template - friend class PmrInterfaceDeleter; + // Below convertor constructor is only possible with enabled PMR at `function`. + // For now, we decided to comment it out, so that `InterfacePtr` always stays + // within 24-bytes small object optimization, namely without extra memory allocation + // just for the sake of "advanced" deleter (actually chain of casters down to original `Concrete` pointer). + // + // template ::value>> + // PmrInterfaceDeleter(const PmrInterfaceDeleter& other) + // : deleter_{other.get_memory_resource(), [other](Interface* ptr) { + // // Delegate to the down class deleter. + // other.deleter_(static_cast(ptr)); + // }} + // { + // } + // + // Pmr* get_memory_resource() const noexcept + // { + // return deleter_.get_memory_resource(); + // } + // + // private: + // template + // friend class PmrInterfaceDeleter; - cetl::pmr::function deleter_; +private: + cetl::pmr::function deleter_; }; // PmrInterfaceDeleter -template -using InterfacePtr = std::unique_ptr>; +template +using InterfacePtr = std::unique_ptr>; class InterfaceFactory final { @@ -69,20 +75,83 @@ class InterfaceFactory final InterfaceFactory() = delete; template - static auto make_unique(PmrAllocator alloc, Args&&... args) - -> InterfacePtr> + CETL_NODISCARD static InterfacePtr make_unique(PmrAllocator alloc, Args&&... args) + { + // Allocate memory for the concrete object. + // Then try to construct it in-place - it could potentially throw, + // so RAII will deallocate the memory BUT won't try to destroy the uninitialized object! + // + ConcreteRaii concrete_raii{alloc}; + if (auto* const concrete_ptr = concrete_raii.get()) + { + concrete_raii.construct(std::forward(args)...); + } + + // Everything is good, so now we can move ownership of the concrete object to the interface pointer. + // + return InterfacePtr{concrete_raii.release(), PmrInterfaceDeleter{alloc, 1}}; + } + +private: + template + class ConcreteRaii final { using Concrete = typename PmrAllocator::value_type; - using Pmr = std::remove_pointer_t; - InterfacePtr concrete_ptr{alloc.allocate(1), PmrInterfaceDeleter{alloc, 1}}; - if (nullptr != concrete_ptr) + public: + ConcreteRaii(PmrAllocator& pmr_allocator) + : concrete_{pmr_allocator.allocate(1)} + , constructed_{false} + , pmr_allocator_{pmr_allocator} + { + } + ~ConcreteRaii() { - alloc.construct(concrete_ptr.get(), std::forward(args)...); + if (nullptr != concrete_) + { + if (constructed_) + { + concrete_->~Concrete(); + } + pmr_allocator_.deallocate(concrete_, 1); + } } + ConcreteRaii(const ConcreteRaii&) = delete; + ConcreteRaii(ConcreteRaii&&) noexcept = delete; + ConcreteRaii& operator=(const ConcreteRaii&) = delete; + ConcreteRaii& operator=(ConcreteRaii&&) noexcept = delete; - return concrete_ptr; - } + template + void construct(Args&&... args) + { + CETL_DEBUG_ASSERT(constructed_ == false, ""); + + if (nullptr != concrete_) + { + pmr_allocator_.construct(concrete_, std::forward(args)...); + constructed_ = true; + } + } + + Concrete* get() const noexcept + { + return concrete_; + } + + Concrete* release() + { + constructed_ = false; + Concrete* result{nullptr}; + std::swap(result, concrete_); + return result; + } + + private: + Concrete* concrete_; + bool constructed_; + PmrAllocator& pmr_allocator_; + + }; // ConcreteRaii }; // InterfaceFactory