Commit: 2aff45146f1464ba8899368ad004522cb6a1a98c
Author: Jacques Lucke
Date: Wed Aug 19 16:44:53 2020 +0200
Branches: master
https://developer.blender.org/rB2aff45146f1464ba8899368ad004522cb6a1a98c
BLI: improve exception safety of Vector, Array and Stack
Using C++ exceptions in Blender is difficult, due to the large
number of C functions in the call stack. However, C++ data
structures in blenlib should at least try to be exception safe,
so that they can be used if someone wants to use exceptions
in some isolated area.
This patch improves the exception safety of the Vector, Array
and Stack data structure. This is mainly achieved by reordering
some lines and doing some explicit exception handling.
I don't expect performance of common operations to be affected
by this change.
The three containers are supposed to provide at least the
basic exception guarantee for most methods (except for e.g.
`*_unchecked` methods). So, resources should not leak when
the contained type throws an exception.
I also added new unit tests that test the exception handling
in various cases.
===================================================================
M source/blender/blenlib/BLI_array.hh
M source/blender/blenlib/BLI_memory_utils.hh
M source/blender/blenlib/BLI_stack.hh
M source/blender/blenlib/BLI_vector.hh
M source/blender/blenlib/CMakeLists.txt
M source/blender/blenlib/tests/BLI_array_test.cc
A source/blender/blenlib/tests/BLI_exception_safety_test_utils.hh
M source/blender/blenlib/tests/BLI_memory_utils_test.cc
M source/blender/blenlib/tests/BLI_stack_cxx_test.cc
M source/blender/blenlib/tests/BLI_vector_test.cc
===================================================================
diff --git a/source/blender/blenlib/BLI_array.hh
b/source/blender/blenlib/BLI_array.hh
index 9d09bb3559e..0ffe6b9a750 100644
--- a/source/blender/blenlib/BLI_array.hh
+++ b/source/blender/blenlib/BLI_array.hh
@@ -77,32 +77,39 @@ class Array {
/**
* By default an empty array is created.
*/
- Array()
+ Array(Allocator allocator = {}) noexcept : allocator_(allocator)
{
data_ = inline_buffer_;
size_ = 0;
}
+ Array(NoExceptConstructor, Allocator allocator = {}) noexcept :
Array(allocator)
+ {
+ }
+
/**
* Create a new array that contains copies of all values.
*/
template<typename U, typename std::enable_if_t<std::is_convertible_v<U, T>>
* = nullptr>
- Array(Span<U> values, Allocator allocator = {}) : allocator_(allocator)
+ Array(Span<U> values, Allocator allocator = {}) :
Array(NoExceptConstructor(), allocator)
{
- size_ = values.size();
- data_ = this->get_buffer_for_size(values.size());
- uninitialized_convert_n<U, T>(values.data(), size_, data_);
+ const int64_t size = values.size();
+ data_ = this->get_buffer_for_size(size);
+ uninitialized_convert_n<U, T>(values.data(), size, data_);
+ size_ = size;
}
/**
* Create a new array that contains copies of all values.
*/
template<typename U, typename std::enable_if_t<std::is_convertible_v<U, T>>
* = nullptr>
- Array(const std::initializer_list<U> &values) : Array(Span<U>(values))
+ Array(const std::initializer_list<U> &values, Allocator allocator = {})
+ : Array(Span<U>(values), allocator)
{
}
- Array(const std::initializer_list<T> &values) : Array(Span<T>(values))
+ Array(const std::initializer_list<T> &values, Allocator allocator = {})
+ : Array(Span<T>(values), allocator)
{
}
@@ -114,23 +121,24 @@ class Array {
* even for non-trivial types. This should not be the default though,
because one can easily mess
* up when dealing with uninitialized memory.
*/
- explicit Array(int64_t size)
+ explicit Array(int64_t size, Allocator allocator = {}) :
Array(NoExceptConstructor(), allocator)
{
- size_ = size;
data_ = this->get_buffer_for_size(size);
default_construct_n(data_, size);
+ size_ = size;
}
/**
* Create a new array with the given size. All values will be initialized by
copying the given
* default.
*/
- Array(int64_t size, const T &value)
+ Array(int64_t size, const T &value, Allocator allocator = {})
+ : Array(NoExceptConstructor(), allocator)
{
BLI_assert(size >= 0);
- size_ = size;
data_ = this->get_buffer_for_size(size);
- uninitialized_fill_n(data_, size_, value);
+ uninitialized_fill_n(data_, size, value);
+ size_ = size;
}
/**
@@ -145,28 +153,28 @@ class Array {
* Usage:
* Array<std::string> my_strings(10, NoInitialization());
*/
- Array(int64_t size, NoInitialization)
+ Array(int64_t size, NoInitialization, Allocator allocator = {})
+ : Array(NoExceptConstructor(), allocator)
{
BLI_assert(size >= 0);
- size_ = size;
data_ = this->get_buffer_for_size(size);
+ size_ = size;
}
Array(const Array &other) : Array(other.as_span(), other.allocator_)
{
}
- Array(Array &&other) noexcept : allocator_(other.allocator_)
+ Array(Array &&other) noexcept(std::is_nothrow_move_constructible_v<T>)
+ : Array(NoExceptConstructor(), other.allocator_)
{
- size_ = other.size_;
-
- if (!other.uses_inline_buffer()) {
- data_ = other.data_;
+ if (other.uses_inline_buffer()) {
+ uninitialized_relocate_n(other.data_, other.size_, data_);
}
else {
- data_ = this->get_buffer_for_size(size_);
- uninitialized_relocate_n(other.data_, size_, data_);
+ data_ = other.data_;
}
+ size_ = other.size_;
other.data_ = other.inline_buffer_;
other.size_ = 0;
@@ -182,24 +190,12 @@ class Array {
Array &operator=(const Array &other)
{
- if (this == &other) {
- return *this;
- }
-
- this->~Array();
- new (this) Array(other);
- return *this;
+ return copy_assign_container(*this, other);
}
- Array &operator=(Array &&other)
+ Array &operator=(Array &&other)
noexcept(std::is_nothrow_move_constructible_v<T>)
{
- if (this == &other) {
- return *this;
- }
-
- this->~Array();
- new (this) Array(std::move(other));
- return *this;
+ return move_assign_container(*this, std::move(other));
}
T &operator[](int64_t index)
diff --git a/source/blender/blenlib/BLI_memory_utils.hh
b/source/blender/blenlib/BLI_memory_utils.hh
index 7216536a884..49076bb1aae 100644
--- a/source/blender/blenlib/BLI_memory_utils.hh
+++ b/source/blender/blenlib/BLI_memory_utils.hh
@@ -162,7 +162,7 @@ void uninitialized_convert_n(const From *src, int64_t n, To
*dst)
int64_t current = 0;
try {
for (; current < n; current++) {
- new (static_cast<void *>(dst + current)) To((To)src[current]);
+ new (static_cast<void *>(dst + current))
To(static_cast<To>(src[current]));
}
}
catch (...) {
@@ -409,6 +409,15 @@ template<typename T, int64_t Size = 1> class TypedBuffer {
class NoInitialization {
};
+/**
+ * This can be used to mark a constructor of an object that does not throw
exceptions. Other
+ * constructors can delegate to this constructor to make sure that the object
lifetime starts.
+ * With this, the destructor of the object will be called, even when the
remaining constructor
+ * throws.
+ */
+class NoExceptConstructor {
+};
+
/**
* Helper variable that checks if a pointer type can be converted into another
pointer type without
* issues. Possible issues are casting away const and casting a pointer to a
child class.
@@ -427,4 +436,49 @@ inline constexpr int64_t
default_inline_buffer_capacity(size_t element_size)
return (static_cast<int64_t>(element_size) < 100) ? 4 : 0;
}
+/**
+ * This can be used by containers to implement an exception-safe
copy-assignment-operator.
+ * It assumes that the container has an exception safe copy constructor and an
exception-safe
+ * move-assignment-operator.
+ */
+template<typename Container> Container ©_assign_container(Container &dst,
const Container &src)
+{
+ if (&src == &dst) {
+ return dst;
+ }
+
+ Container container_copy{src};
+ dst = std::move(container_copy);
+ return dst;
+}
+
+/**
+ * This can be used by containers to implement an exception-safe
move-assignment-operator.
+ * It assumes that the container has an exception-safe move-constructor and a
noexcept constructor
+ * tagged with the NoExceptConstructor tag.
+ */
+template<typename Container>
+Container &move_assign_container(Container &dst, Container &&src) noexcept(
+ std::is_nothrow_move_constructible_v<Container>)
+{
+ if (&dst == &src) {
+ return dst;
+ }
+
+ dst.~Container();
+ if constexpr (std::is_nothrow_move_constructible_v<Container>) {
+ new (&dst) Container(std::move(src));
+ }
+ else {
+ try {
+ new (&dst) Container(std::move(src));
+ }
+ catch (...) {
+ new (&dst) Container(NoExceptConstructor());
+ throw;
+ }
+ }
+ return dst;
+}
+
} // namespace blender
diff --git a/source/blender/blenlib/BLI_stack.hh
b/source/blender/blenlib/BLI_stack.hh
index 8eca356ec54..a463ac102f1 100644
--- a/source/blender/blenlib/BLI_stack.hh
+++ b/source/blender/blenlib/BLI_stack.hh
@@ -117,7 +117,7 @@ class Stack {
/**
* Initialize an empty stack. No heap allocation is done.
*/
- Stack(Allocator allocator = {}) : allocator_(allocator)
+ Stack(Allocator allocator = {}) noexcept : allocator_(allocator)
{
inline_chunk_.below = nullptr;
inline_chunk_.above = nullptr;
@@ -129,11 +129,15 @@ class Stack {
size_ = 0;
}
+ Stack(NoExceptConstructor, Allocator allocator = {}) noexcept :
Stack(allocator)
+ {
+ }
+
/**
* Create a new stack that contains the given elements. The values are
pushed to the stack in
* the order they are in the array.
*/
- Stack(Span<T> values) : Stack()
+ Stack(Span<T> values, Allocator allocator = {}) :
Stack(NoExceptConstructor(), allocator)
{
this->push_multiple(values);
}
@@ -147,11 +151,12 @@ class Stack {
* assert(stack.pop() == 6);
* assert(stack.pop() == 5);
*/
- Stack(const std::initializer_list<T> &values) : Stack(Span<T>(values))
+ Stack(const std::initializer_list<T> &values, Allocator allocator = {})
+ : Stack(Span<T>(values), allocator)
{
}
- Stack(const Stack &other) : Stack(other.allocator_)
+ Stack(const Stack &other) : Stack(NoExceptConstructor(), other.allocator_)
{
for (const Chunk *chunk = &other.inline_chunk_; chunk; chunk =
chunk->above) {
const T *begin = chunk->begin;
@@ -160,7 +165,8 @@ class Stack {
}
}
- Stack(Stack &&other) noexcept : Stack(other.allocator_)
+ Stack(Stack &&other) noexcept(std::is_nothrow_move_constructible_v<T>)
+ : Stack(NoExceptConstructor(), other.allocator_)
{
uninitialized_relocate_n<T>(
other.inline_buffer_, std::min(other.size_, InlineBufferCapacity),
inline_buffer_);
@@ -197,28 +203,14 @@ class Stack {
}
}
- Stack &operator=(const Stack &stack)
+ Stack &operator=(const Stack &other)
{
- if (this == &stack) {
- return *this;
- }
-
- this->~Stack();
- new (this) Stack(stack);
-
- return *this;
+ return copy_assign_container(*this, other);
}
- Stack &operator=(Stack &&stack)
+ Stack &operator=(Stack &&other)
{
- if (this == &stack) {
- return *this;
- }
-
- this->~Stack();
- new (this) Stack(std::move(stack));
-
- return *this;
+ return move_assign_container(*this, std::move(other));
}
/**
@@ -226,21 +218,26 @@ class Stack {
*/
void push(const T &value)
{
- if (top_ == top_chunk_->capacity_end) {
- this->activate_next_chunk(1);
- }
- new (top_) T(value);
- top_++;
- size_++;
+ this->push_as(value);
}
void push(T &&value)
+ {
+ this->push_as(std::move(value));
+ }
+ template<typename ForwardT> void push_as(ForwardT &&value)
{
if (top_ == top_chunk_->capacity_end) {
this->activate_next_chunk(1);
}
- new (top_) T(std::move(value));
@@ Diff output truncated at 10240 characters. @@
_______________________________________________
Bf-blender-cvs mailing list
[email protected]
https://lists.blender.org/mailman/listinfo/bf-blender-cvs
