[Bf-blender-cvs] [572c48cf986] master: BLI: improve exception safety of memory utils
Jacques Lucke
noreply at git.blender.org
Mon Jul 6 11:09:41 CEST 2020
Commit: 572c48cf98601c66eebec27bb40cfc7e05ea341c
Author: Jacques Lucke
Date: Mon Jul 6 09:08:53 2020 +0200
Branches: master
https://developer.blender.org/rB572c48cf98601c66eebec27bb40cfc7e05ea341c
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.
===================================================================
M source/blender/blenlib/BLI_memory_utils.hh
A tests/gtests/blenlib/BLI_memory_utils_test.cc
M tests/gtests/blenlib/BLI_vector_test.cc
M tests/gtests/blenlib/CMakeLists.txt
M tests/gtests/functions/FN_cpp_type_test.cc
===================================================================
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")
B
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list