pw_allocator: Make Layout constructor explicit

This CL adds the explicit keyword to the Layout constructor. Without
this, passing a size_t to a method that requires a Layput would silently
convert it with a default alignment. This is potentially error-prone, as
calls passing just an alignment would build but not behave as exepcted.

Change-Id: I7b35dc3881ba93934c019b0c852cbcfbf969aaa3
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/211915
Lint: Lint 🤖 <[email protected]>
Commit-Queue: Aaron Green <[email protected]>
Presubmit-Verified: CQ Bot Account <[email protected]>
Reviewed-by: Taylor Cramer <[email protected]>

pw_allocator/BUILD.bazel

@@ -639,6 +639,15 @@ pw_cc_test(
     ],
 )
 
+pw_cc_test(
+    name = "layout_test",
+    srcs = ["layout_test.cc"],
+    deps = [
+        ":deallocator",
+        "//pw_unit_test",
+    ],
+)
+
 pw_cc_test(
     name = "libc_allocator_test",
     srcs = [

pw_allocator/BUILD.gn

@@ -505,6 +505,11 @@ pw_test("last_fit_block_allocator_test") {
   sources = [ "last_fit_block_allocator_test.cc" ]
 }
 
+pw_test("layout_test") {
+  deps = [ ":deallocator" ]
+  sources = [ "layout_test.cc" ]
+}
+
 pw_test("libc_allocator_test") {
   deps = [ ":libc_allocator" ]
   sources = [ "libc_allocator_test.cc" ]
@@ -585,6 +590,7 @@ pw_test_group("tests") {
     ":fragmentation_test",
     ":freelist_heap_test",
     ":last_fit_block_allocator_test",
+    ":layout_test",
     ":libc_allocator_test",
     ":null_allocator_test",
     ":typed_pool_test",

pw_allocator/CMakeLists.txt

@@ -583,6 +583,16 @@ pw_add_test(pw_allocator.freelist_heap_test
     pw_allocator
 )
 
+pw_add_test(pw_allocator.layout_test
+  SOURCES
+    layout_test.cc
+  PRIVATE_DEPS
+    pw_allocator.deallocator
+  GROUPS
+    modules
+    pw_allocator
+)
+
 pw_add_test(pw_allocator.last_fit_block_allocator_test
   SOURCES
     last_fit_block_allocator_test.cc

pw_allocator/allocator_test.cc

@@ -60,11 +60,11 @@ TEST(AllocatorTest, ResizeSame) {
 TEST(AllocatorTest, ReallocateNull) {
   AllocatorForTest allocator;
   constexpr Layout old_layout = Layout::Of<uint32_t>();
-  size_t new_size = old_layout.size();
-  void* new_ptr = allocator.Reallocate(nullptr, new_size);
+  constexpr Layout new_layout(old_layout.size(), old_layout.alignment());
+  void* new_ptr = allocator.Reallocate(nullptr, new_layout);
 
   // Resize should fail and Reallocate should call Allocate.
-  EXPECT_EQ(allocator.allocate_size(), new_size);
+  EXPECT_EQ(allocator.allocate_size(), new_layout.size());
 
   // Deallocate should not be called.
   EXPECT_EQ(allocator.deallocate_ptr(), nullptr);
@@ -82,8 +82,8 @@ TEST(AllocatorTest, ReallocateZeroNewSize) {
   ASSERT_NE(ptr, nullptr);
   allocator.ResetParameters();
 
-  size_t new_size = 0;
-  void* new_ptr = allocator.Reallocate(ptr, new_size);
+  constexpr Layout new_layout(0, old_layout.alignment());
+  void* new_ptr = allocator.Reallocate(ptr, new_layout);
 
   // Reallocate does not call Resize, Allocate, or Deallocate.
   EXPECT_EQ(allocator.resize_ptr(), nullptr);
@@ -131,13 +131,13 @@ TEST(AllocatorTest, ReallocateSmaller) {
   ASSERT_NE(ptr, nullptr);
   allocator.ResetParameters();
 
-  size_t new_size = sizeof(uint32_t);
-  void* new_ptr = allocator.Reallocate(ptr, new_size);
+  constexpr Layout new_layout(sizeof(uint32_t), old_layout.alignment());
+  void* new_ptr = allocator.Reallocate(ptr, new_layout);
 
   // Reallocate should call Resize.
   EXPECT_EQ(allocator.resize_ptr(), ptr);
   EXPECT_EQ(allocator.resize_old_size(), old_layout.size());
-  EXPECT_EQ(allocator.resize_new_size(), new_size);
+  EXPECT_EQ(allocator.resize_new_size(), new_layout.size());
 
   // Allocate should not be called.
   EXPECT_EQ(allocator.allocate_size(), 0U);
@@ -166,16 +166,16 @@ TEST(AllocatorTest, ReallocateLarger) {
   ASSERT_NE(next, nullptr);
   allocator.ResetParameters();
 
-  size_t new_size = sizeof(uint32_t[3]);
-  void* new_ptr = allocator.Reallocate(ptr, new_size);
+  constexpr Layout new_layout(sizeof(uint32_t[3]), old_layout.alignment());
+  void* new_ptr = allocator.Reallocate(ptr, new_layout);
 
   // Reallocate should call Resize.
   EXPECT_EQ(allocator.resize_ptr(), ptr);
   EXPECT_EQ(allocator.resize_old_size(), old_layout.size());
-  EXPECT_EQ(allocator.resize_new_size(), new_size);
+  EXPECT_EQ(allocator.resize_new_size(), new_layout.size());
 
   // Resize should fail and Reallocate should call Allocate.
-  EXPECT_EQ(allocator.allocate_size(), new_size);
+  EXPECT_EQ(allocator.allocate_size(), new_layout.size());
 
   // Deallocate should also be called.
   EXPECT_EQ(allocator.deallocate_ptr(), ptr);

pw_allocator/layout_test.cc

@@ -0,0 +1,96 @@
+// Copyright 2024 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+
+#include "pw_allocator/layout.h"
+
+#include <cstddef>
+
+#include "pw_result/result.h"
+#include "pw_status/status.h"
+#include "pw_unit_test/framework.h"
+
+namespace {
+
+using ::pw::allocator::Layout;
+
+TEST(LayoutTest, DefaultConstructor) {
+  Layout layout;
+  EXPECT_EQ(layout.size(), 0U);
+  EXPECT_EQ(layout.alignment(), alignof(std::max_align_t));
+}
+
+TEST(LayoutTest, SizeOnlyConstructor) {
+  constexpr size_t kSize = 512;
+  Layout layout(kSize);
+  EXPECT_EQ(layout.size(), kSize);
+  EXPECT_EQ(layout.alignment(), alignof(std::max_align_t));
+}
+
+TEST(LayoutTest, FullConstructor) {
+  constexpr size_t kSize = 2048;
+  constexpr size_t kAlignment = 4;
+  Layout layout(kSize, kAlignment);
+  EXPECT_EQ(layout.size(), kSize);
+  EXPECT_EQ(layout.alignment(), kAlignment);
+}
+
+TEST(LayoutTest, ConstructUsingInitializer) {
+  constexpr size_t kSize = 1024;
+  constexpr size_t kAlignment = 8;
+  Layout layout = {kSize, kAlignment};
+  EXPECT_EQ(layout.size(), kSize);
+  EXPECT_EQ(layout.alignment(), kAlignment);
+}
+
+TEST(LayoutTest, ConstructFromType) {
+  struct Values {
+    uint8_t u8;
+    uint16_t u16;
+    uint32_t u32;
+  };
+  Layout layout = Layout::Of<Values>();
+  EXPECT_EQ(layout.size(), sizeof(Values));
+  EXPECT_EQ(layout.alignment(), alignof(Values));
+}
+
+TEST(LayoutTest, Extend) {
+  constexpr size_t kSize1 = 2048;
+  constexpr size_t kSize2 = 1024;
+  constexpr size_t kAlignment = 2;
+  Layout layout1 = {kSize1, kAlignment};
+  EXPECT_EQ(layout1.size(), kSize1);
+  EXPECT_EQ(layout1.alignment(), kAlignment);
+
+  Layout layout2 = layout1.Extend(kSize2);
+  EXPECT_EQ(layout2.size(), kSize1 + kSize2);
+  EXPECT_EQ(layout2.alignment(), kAlignment);
+}
+
+TEST(LayoutTest, UnwrapOk) {
+  constexpr size_t kSize = 1024;
+  constexpr size_t kAlignment = 8;
+  pw::Result<Layout> result = Layout(kSize, kAlignment);
+  Layout layout = Layout::Unwrap(result);
+  EXPECT_EQ(layout.size(), kSize);
+  EXPECT_EQ(layout.alignment(), kAlignment);
+}
+
+TEST(LayoutTest, UnwrapError) {
+  pw::Result<Layout> result = pw::Status::Unimplemented();
+  Layout layout = Layout::Unwrap(result);
+  EXPECT_EQ(layout.size(), 0U);
+  EXPECT_EQ(layout.alignment(), alignof(std::max_align_t));
+}
+
+}  // namespace

pw_allocator/libc_allocator_test.cc

@@ -62,8 +62,8 @@ TEST(LibCAllocatorTest, Reallocate) {
   constexpr Layout old_layout = Layout::Of<uint32_t[4]>();
   void* ptr = allocator.Allocate(old_layout);
   ASSERT_NE(ptr, nullptr);
-  constexpr size_t new_size = sizeof(uint32_t[3]);
-  void* new_ptr = allocator.Reallocate(ptr, new_size);
+  constexpr Layout new_layout(sizeof(uint32_t[3]), old_layout.alignment());
+  void* new_ptr = allocator.Reallocate(ptr, new_layout);
   ASSERT_NE(new_ptr, nullptr);
   allocator.Deallocate(new_ptr);
 }

pw_allocator/public/pw_allocator/layout.h

@@ -38,8 +38,10 @@ namespace pw::allocator {
 /// @endcode
 class Layout {
  public:
-  constexpr Layout() = default;
-  constexpr Layout(size_t size, size_t alignment = alignof(std::max_align_t))
+  constexpr Layout() : Layout(0) {}
+  constexpr explicit Layout(size_t size)
+      : Layout(size, alignof(std::max_align_t)) {}
+  constexpr Layout(size_t size, size_t alignment)
       : size_(size), alignment_(alignment) {}
 
   /// Creates a Layout for the given type.
@@ -59,12 +61,12 @@ class Layout {
     return Layout(size, alignment_);
   }
 
-  size_t size() const { return size_; }
-  size_t alignment() const { return alignment_; }
+  constexpr size_t size() const { return size_; }
+  constexpr size_t alignment() const { return alignment_; }
 
  private:
-  size_t size_ = 0;
-  size_t alignment_ = 1;
+  size_t size_;
+  size_t alignment_;
 };
 
 inline bool operator==(const Layout& lhs, const Layout& rhs) {