BLI: improve exception safety of memory utils

Even if we do not use exception in many places in Blender, our core C++ library
should become exception safe. Otherwise, we don't even have the option
to work with exceptions if we decide to do so.

5 files changed, 204 insertions, 25 deletions

diff --git a/source/blender/blenlib/BLI_memory_utils.hh b/source/blender/blenlib/BLI_memory_utils.hh
index 1c0193e2756..4e7234b85fb 100644
--- a/source/blender/blenlib/BLI_memory_utils.hh
+++ b/source/blender/blenlib/BLI_memory_utils.hh
@@ -19,6 +19,9 @@
 
 /** \file
  * \ingroup bli
+ * Some of the functions below have very similar alternatives in the standard library. However, it
+ * is rather annoying to use those when debugging. Therefore, some more specialized and easier to
+ * debug functions are provided here.
  */
 
 #include <memory>
@@ -29,52 +32,68 @@
 namespace blender {
 
 /**
- * Call the default constructor on n consecutive elements. For trivially constructible types, this
- * does nothing.
+ * Call the destructor on n consecutive values. For trivially destructible types, this does
+ * nothing.
+ *
+ * Exception Safety: Destructors shouldn't throw exceptions.
  *
  * Before:
- *  ptr: uninitialized
- * After:
  *  ptr: initialized
+ * After:
+ *  ptr: uninitialized
  */
-template<typename T> void default_construct_n(T *ptr, uint n)
+template<typename T> void destruct_n(T *ptr, uint n)
 {
+  static_assert(std::is_nothrow_destructible_v<T>,
+                "This should be true for all types. Destructors are noexcept by default.");
+
   /* This is not strictly necessary, because the loop below will be optimized away anyway. It is
    * nice to make behavior this explicitly, though. */
-  if (std::is_trivially_constructible<T>::value) {
+  if (std::is_trivially_destructible<T>::value) {
     return;
   }
 
   for (uint i = 0; i < n; i++) {
-    new ((void *)(ptr + i)) T;
+    ptr[i].~T();
   }
 }
 
 /**
- * Call the destructor on n consecutive values. For trivially destructible types, this does
- * nothing.
+ * Call the default constructor on n consecutive elements. For trivially constructible types, this
+ * does nothing.
+ *
+ * Exception Safety: Strong.
  *
  * Before:
- *  ptr: initialized
- * After:
  *  ptr: uninitialized
+ * After:
+ *  ptr: initialized
  */
-template<typename T> void destruct_n(T *ptr, uint n)
+template<typename T> void default_construct_n(T *ptr, uint n)
 {
   /* This is not strictly necessary, because the loop below will be optimized away anyway. It is
    * nice to make behavior this explicitly, though. */
-  if (std::is_trivially_destructible<T>::value) {
+  if (std::is_trivially_constructible<T>::value) {
     return;
   }
 
-  for (uint i = 0; i < n; i++) {
-    ptr[i].~T();
+  uint current = 0;
+  try {
+    for (; current < n; current++) {
+      new ((void *)(ptr + current)) T;
+    }
+  }
+  catch (...) {
+    destruct_n(ptr, current);
+    throw;
   }
 }
 
 /**
  * Copy n values from src to dst.
  *
+ * Exception Safety: Basic.
+ *
  * Before:
  *  src: initialized
  *  dst: initialized
@@ -92,6 +111,8 @@ template<typename T> void initialized_copy_n(const T *src, uint n, T *dst)
 /**
  * Copy n values from src to dst.
  *
+ * Exception Safety: Strong.
+ *
  * Before:
  *  src: initialized
  *  dst: uninitialized
@@ -101,14 +122,23 @@ template<typename T> void initialized_copy_n(const T *src, uint n, T *dst)
  */
 template<typename T> void uninitialized_copy_n(const T *src, uint n, T *dst)
 {
-  for (uint i = 0; i < n; i++) {
-    new ((void *)(dst + i)) T(src[i]);
+  uint current = 0;
+  try {
+    for (; current < n; current++) {
+      new ((void *)(dst + current)) T(src[current]);
+    }
+  }
+  catch (...) {
+    destruct_n(dst, current);
+    throw;
   }
 }
 
 /**
  * Move n values from src to dst.
  *
+ * Exception Safety: Basic.
+ *
  * Before:
  *  src: initialized
  *  dst: initialized
@@ -126,6 +156,8 @@ template<typename T> void initialized_move_n(T *src, uint n, T *dst)
 /**
  * Move n values from src to dst.
  *
+ * Exception Safety: Basic.
+ *
  * Before:
  *  src: initialized
  *  dst: uninitialized
@@ -135,8 +167,19 @@ template<typename T> void initialized_move_n(T *src, uint n, T *dst)
  */
 template<typename T> void uninitialized_move_n(T *src, uint n, T *dst)
 {
-  for (uint i = 0; i < n; i++) {
-    new ((void *)(dst + i)) T(std::move(src[i]));
+  static_assert(std::is_nothrow_move_constructible_v<T>,
+                "Ideally, all types should have this property. We might have to remove this "
+                "limitation of a real reason comes up.");
+
+  uint current = 0;
+  try {
+    for (; current < n; current++) {
+      new ((void *)(dst + current)) T(std::move(src[current]));
+    }
+  }
+  catch (...) {
+    destruct_n(dst, current);
+    throw;
   }
 }
 
@@ -144,6 +187,8 @@ template<typename T> void uninitialized_move_n(T *src, uint n, T *dst)
  * Relocate n values from src to dst. Relocation is a move followed by destruction of the src
  * value.
  *
+ * Exception Safety: Basic.
+ *
  * Before:
  *  src: initialized
  *  dst: initialized
@@ -161,6 +206,8 @@ template<typename T> void initialized_relocate_n(T *src, uint n, T *dst)
  * Relocate n values from src to dst. Relocation is a move followed by destruction of the src
  * value.
  *
+ * Exception Safety: Basic.
+ *
  * Before:
  *  src: initialized
  *  dst: uninitialized
@@ -177,6 +224,8 @@ template<typename T> void uninitialized_relocate_n(T *src, uint n, T *dst)
 /**
  * Copy the value to n consecutive elements.
  *
+ * Exception Safety: Basic.
+ *
  * Before:
  *  dst: initialized
  * After:
@@ -192,6 +241,8 @@ template<typename T> void initialized_fill_n(T *dst, uint n, const T &value)
 /**
  * Copy the value to n consecutive elements.
  *
+ *  Exception Safety: Strong.
+ *
  * Before:
  *  dst: uninitialized
  * After:
@@ -199,8 +250,15 @@ template<typename T> void initialized_fill_n(T *dst, uint n, const T &value)
  */
 template<typename T> void uninitialized_fill_n(T *dst, uint n, const T &value)
 {
-  for (uint i = 0; i < n; i++) {
-    new ((void *)(dst + i)) T(value);
+  uint current = 0;
+  try {
+    for (; current < n; current++) {
+      new ((void *)(dst + current)) T(value);
+    }
+  }
+  catch (...) {
+    destruct_n(dst, current);
+    throw;
   }
 }
 
diff --git a/tests/gtests/blenlib/BLI_memory_utils_test.cc b/tests/gtests/blenlib/BLI_memory_utils_test.cc
new file mode 100644
index 00000000000..aec57d02819
--- /dev/null
+++ b/tests/gtests/blenlib/BLI_memory_utils_test.cc
@@ -0,0 +1,120 @@
+#include "BLI_memory_utils.hh"
+#include "BLI_strict_flags.h"
+#include "testing/testing.h"
+
+namespace blender {
+
+struct MyValue {
+  static inline int alive = 0;
+
+  MyValue()
+  {
+    if (alive == 15) {
+      throw std::exception();
+    }
+
+    alive++;
+  }
+
+  MyValue(const MyValue &other)
+  {
+    if (alive == 15) {
+      throw std::exception();
+    }
+
+    alive++;
+  }
+
+  ~MyValue()
+  {
+    alive--;
+  }
+};
+
+TEST(memory_utils, DefaultConstructN_ActuallyCallsConstructor)
+{
+  constexpr int amount = 10;
+  TypedBuffer<MyValue, amount> buffer;
+
+  EXPECT_EQ(MyValue::alive, 0);
+  default_construct_n(buffer.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, amount);
+  destruct_n(buffer.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, 0);
+}
+
+TEST(memory_utils, DefaultConstructN_StrongExceptionSafety)
+{
+  constexpr int amount = 20;
+  TypedBuffer<MyValue, amount> buffer;
+
+  EXPECT_EQ(MyValue::alive, 0);
+  EXPECT_THROW(default_construct_n(buffer.ptr(), amount), std::exception);
+  EXPECT_EQ(MyValue::alive, 0);
+}
+
+TEST(memory_utils, UninitializedCopyN_ActuallyCopies)
+{
+  constexpr int amount = 5;
+  TypedBuffer<MyValue, amount> buffer1;
+  TypedBuffer<MyValue, amount> buffer2;
+
+  EXPECT_EQ(MyValue::alive, 0);
+  default_construct_n(buffer1.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, amount);
+  uninitialized_copy_n(buffer1.ptr(), amount, buffer2.ptr());
+  EXPECT_EQ(MyValue::alive, 2 * amount);
+  destruct_n(buffer1.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, amount);
+  destruct_n(buffer2.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, 0);
+}
+
+TEST(memory_utils, UninitializedCopyN_StrongExceptionSafety)
+{
+  constexpr int amount = 10;
+  TypedBuffer<MyValue, amount> buffer1;
+  TypedBuffer<MyValue, amount> buffer2;
+
+  EXPECT_EQ(MyValue::alive, 0);
+  default_construct_n(buffer1.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, amount);
+  EXPECT_THROW(uninitialized_copy_n(buffer1.ptr(), amount, buffer2.ptr()), std::exception);
+  EXPECT_EQ(MyValue::alive, amount);
+  destruct_n(buffer1.ptr(), amount);
+  EXPECT_EQ(MyValue::alive, 0);
+}
+
+TEST(memory_utils, UninitializedFillN_ActuallyCopies)
+{
+  constexpr int amount = 10;
+  TypedBuffer<MyValue, amount> buffer;
+
+  EXPECT_EQ(MyValue::alive, 0);
+  {
+    MyValue value;
+    EXPECT_EQ(MyValue::alive, 1);
+    uninitialized_fill_n(buffer.ptr(), amount, value);
+    EXPECT_EQ(MyValue::alive, 1 + amount);
+    destruct_n(buffer.ptr(), amount);
+    EXPECT_EQ(MyValue::alive, 1);
+  }
+  EXPECT_EQ(MyValue::alive, 0);
+}
+
+TEST(memory_utils, UninitializedFillN_StrongExceptionSafety)
+{
+  constexpr int amount = 20;
+  TypedBuffer<MyValue, amount> buffer;
+
+  EXPECT_EQ(MyValue::alive, 0);
+  {
+    MyValue value;
+    EXPECT_EQ(MyValue::alive, 1);
+    EXPECT_THROW(uninitialized_fill_n(buffer.ptr(), amount, value), std::exception);
+    EXPECT_EQ(MyValue::alive, 1);
+  }
+  EXPECT_EQ(MyValue::alive, 0);
+}
+
+}  // namespace blender
diff --git a/tests/gtests/blenlib/BLI_vector_test.cc b/tests/gtests/blenlib/BLI_vector_test.cc
index ea4711ca015..bd72a67746a 100644
--- a/tests/gtests/blenlib/BLI_vector_test.cc
+++ b/tests/gtests/blenlib/BLI_vector_test.cc
@@ -499,7 +499,7 @@ class TypeConstructMock {
   {
   }
 
-  TypeConstructMock(TypeConstructMock &&other) : move_constructed(true)
+  TypeConstructMock(TypeConstructMock &&other) noexcept : move_constructed(true)
   {
   }
 
@@ -513,7 +513,7 @@ class TypeConstructMock {
     return *this;
   }
 
-  TypeConstructMock &operator=(TypeConstructMock &&other)
+  TypeConstructMock &operator=(TypeConstructMock &&other) noexcept
   {
     if (this == &other) {
       return *this;
diff --git a/tests/gtests/blenlib/CMakeLists.txt b/tests/gtests/blenlib/CMakeLists.txt
index ba493b22b42..0831024556b 100644
--- a/tests/gtests/blenlib/CMakeLists.txt
+++ b/tests/gtests/blenlib/CMakeLists.txt
@@ -63,6 +63,7 @@ BLENDER_TEST(BLI_math_geom "bf_blenlib")
 BLENDER_TEST(BLI_math_matrix "bf_blenlib")
 BLENDER_TEST(BLI_math_vector "bf_blenlib")
 BLENDER_TEST(BLI_memiter "bf_blenlib")
+BLENDER_TEST(BLI_memory_utils "bf_blenlib")
 BLENDER_TEST(BLI_path_util "${BLI_path_util_extra_libs}")
 BLENDER_TEST(BLI_polyfill_2d "bf_blenlib")
 BLENDER_TEST(BLI_set "bf_blenlib")
diff --git a/tests/gtests/functions/FN_cpp_type_test.cc b/tests/gtests/functions/FN_cpp_type_test.cc
index f6ae0877ed1..1297ca471b7 100644
--- a/tests/gtests/functions/FN_cpp_type_test.cc
+++ b/tests/gtests/functions/FN_cpp_type_test.cc
@@ -50,7 +50,7 @@ struct TestType {
     other.value = copy_constructed_from_value;
   }
 
-  TestType(TestType &&other)
+  TestType(TestType &&other) noexcept
   {
     value = move_constructed_value;
     other.value = move_constructed_from_value;
@@ -63,7 +63,7 @@ struct TestType {
     return *this;
   }
 
-  TestType &operator=(TestType &&other)
+  TestType &operator=(TestType &&other) noexcept
   {
     value = move_assigned_value;
     other.value = move_assigned_from_value;