diff options
author | Jacques Lucke <jacques@blender.org> | 2020-07-06 10:08:53 +0300 |
---|---|---|
committer | Jacques Lucke <jacques@blender.org> | 2020-07-06 10:08:53 +0300 |
commit | 572c48cf98601c66eebec27bb40cfc7e05ea341c (patch) | |
tree | ff2c056d119a6af0fae7c4d5cdef674eb969f0ae | |
parent | 703a73fa846531a0888c8f99489c8e213f5c5d81 (diff) |
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.
-rw-r--r-- | source/blender/blenlib/BLI_memory_utils.hh | 100 | ||||
-rw-r--r-- | tests/gtests/blenlib/BLI_memory_utils_test.cc | 120 | ||||
-rw-r--r-- | tests/gtests/blenlib/BLI_vector_test.cc | 4 | ||||
-rw-r--r-- | tests/gtests/blenlib/CMakeLists.txt | 1 | ||||
-rw-r--r-- | tests/gtests/functions/FN_cpp_type_test.cc | 4 |
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; |