From 2c0208dad4040eae4d4a2c773a72dfb3ead9d8ea Mon Sep 17 00:00:00 2001 From: Matthew Sedam Date: Mon, 28 Oct 2024 21:07:49 +0000 Subject: [PATCH] pw_allocator: Properly default initialize arrays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL default initializes arrays instead of value initializing arrays in NewArray. This CL also fixes a small type comparison error in UniquePtr. Change-Id: Ieceaf222847e980454c7240ee20269d2b6968c62 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/244772 Docs-Not-Needed: Matthew Sedam Reviewed-by: Taylor Cramer Commit-Queue: Taylor Cramer Commit-Queue: Matthew Sedam Docs-Not-Needed: Taylor Cramer Lint: Lint 🤖 --- pw_allocator/public/pw_allocator/allocator.h | 24 +++------- pw_allocator/public/pw_allocator/unique_ptr.h | 6 ++- pw_allocator/unique_ptr_test.cc | 44 ++++++++++--------- 3 files changed, 35 insertions(+), 39 deletions(-) diff --git a/pw_allocator/public/pw_allocator/allocator.h b/pw_allocator/public/pw_allocator/allocator.h index eb2e49603..f0153ccff 100644 --- a/pw_allocator/public/pw_allocator/allocator.h +++ b/pw_allocator/public/pw_allocator/allocator.h @@ -61,19 +61,11 @@ class Allocator : public Deallocator { /// pointer. /// /// @param[in] size The size of the array to allocate. - /// @param[in] args... Arguments passed to the object constructor. - template - T* NewArray(size_t size, Args&&... args) { + template + T* NewArray(size_t size) { Layout layout(sizeof(T) * size, alignof(T)); - T* ptr = static_cast(Allocate(layout)); - if (ptr == nullptr) { - return nullptr; - } - - for (size_t i = 0; i < size; ++i) { - new (ptr + i) T(args...); - } - return ptr; + void* ptr = Allocate(layout); + return ptr != nullptr ? new (ptr) T[size] : nullptr; } /// Constructs and object of type `T` from the given `args`, and wraps it in a @@ -95,11 +87,9 @@ class Allocator : public Deallocator { /// fails. Callers must check for null before using the `UniquePtr`. /// /// @param[in] size The size of the array to allocate. - /// @param[in] args... Arguments passed to the object constructor. - template - [[nodiscard]] UniquePtr MakeUniqueArray(size_t size, Args&&... args) { - return Deallocator::WrapUniqueArray( - NewArray(size, std::forward(args)...), size); + template + [[nodiscard]] UniquePtr MakeUniqueArray(size_t size) { + return Deallocator::WrapUniqueArray(NewArray(size), size); } /// Modifies the size of an previously-allocated block of memory without diff --git a/pw_allocator/public/pw_allocator/unique_ptr.h b/pw_allocator/public/pw_allocator/unique_ptr.h index e153bf506..3a6fea99d 100644 --- a/pw_allocator/public/pw_allocator/unique_ptr.h +++ b/pw_allocator/public/pw_allocator/unique_ptr.h @@ -83,7 +83,8 @@ class UniquePtr : public allocator::internal::BaseUniquePtr { deallocator_(other.deallocator_), size_(other.size_) { static_assert( - std::is_assignable_v, + std::is_assignable_v::UnderlyingType*>, "Attempted to construct a UniquePtr from a UniquePtr where " "U* is not assignable to T*."); other.Release(); @@ -106,7 +107,8 @@ class UniquePtr : public allocator::internal::BaseUniquePtr { /// ``UniquePtr base = deallocator.MakeUnique();``. template UniquePtr& operator=(UniquePtr&& other) noexcept { - static_assert(std::is_assignable_v, + static_assert(std::is_assignable_v::UnderlyingType*>, "Attempted to assign a UniquePtr to a UniquePtr where " "U* is not assignable to T*."); Reset(); diff --git a/pw_allocator/unique_ptr_test.cc b/pw_allocator/unique_ptr_test.cc index fad65b5ea..7ac435ba4 100644 --- a/pw_allocator/unique_ptr_test.cc +++ b/pw_allocator/unique_ptr_test.cc @@ -152,40 +152,44 @@ TEST_F(UniquePtrTest, DestructorDestroysAndFrees) { EXPECT_EQ(allocator_.deallocate_size(), sizeof(DestructorCounter)); } +class ConstructorCounter { + public: + ConstructorCounter() { ++count_; } + + size_t getCount() { return count_; } + + private: + static size_t count_; +}; +size_t ConstructorCounter::count_ = 0; + TEST_F(UniquePtrTest, ArrayElementsAreConstructed) { constexpr static size_t kArraySize = 5; - size_t count = 0; - class ConstructorCounter { - public: - ConstructorCounter(size_t& count) { ++count; } - }; - EXPECT_EQ(count, 0ul); pw::UniquePtr ptr = - allocator_.MakeUniqueArray(kArraySize, count); + allocator_.MakeUniqueArray(kArraySize); ASSERT_NE(ptr, nullptr); - EXPECT_EQ(count, kArraySize); + EXPECT_EQ(ptr[0].getCount(), kArraySize); } +class DestructorCounter { + public: + ~DestructorCounter() { ++count_; } + + static size_t count_; +}; +size_t DestructorCounter::count_ = 0; + TEST_F(UniquePtrTest, DestructorDestroysAndFreesArray) { constexpr static size_t kArraySize = 5; - size_t count = 0; - class DestructorCounter { - public: - DestructorCounter(size_t& count) : count_(&count) {} - ~DestructorCounter() { (*count_)++; } - - private: - size_t* count_; - }; pw::UniquePtr ptr = - allocator_.MakeUniqueArray(kArraySize, count); + allocator_.MakeUniqueArray(kArraySize); ASSERT_NE(ptr, nullptr); - EXPECT_EQ(count, 0ul); + EXPECT_EQ(DestructorCounter::count_, 0ul); EXPECT_EQ(allocator_.deallocate_size(), 0ul); ptr.Reset(); // Reset the UniquePtr, destroying its contents. - EXPECT_EQ(count, kArraySize); + EXPECT_EQ(DestructorCounter::count_, kArraySize); EXPECT_EQ(allocator_.deallocate_size(), sizeof(DestructorCounter) * kArraySize); }