From 8e18a9984505514a229d66b38fff31d930367968 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Mon, 24 Aug 2020 17:24:13 +0200 Subject: BLI: improve exception safety of Set and Map For more information see rB2aff45146f1464ba8899368ad004522cb6a1a98c. --- source/blender/blenlib/BLI_map_slots.hh | 95 ++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 44 deletions(-) (limited to 'source/blender/blenlib/BLI_map_slots.hh') diff --git a/source/blender/blenlib/BLI_map_slots.hh b/source/blender/blenlib/BLI_map_slots.hh index 25fb92d61a3..c0cb3091a8e 100644 --- a/source/blender/blenlib/BLI_map_slots.hh +++ b/source/blender/blenlib/BLI_map_slots.hh @@ -38,6 +38,19 @@ namespace blender { +template +void initialize_pointer_pair(Src1 &&src1, Src2 &&src2, Dst1 *dst1, Dst2 *dst2) +{ + new ((void *)dst1) Dst1(std::forward(src1)); + try { + new ((void *)dst2) Dst2(std::forward(src2)); + } + catch (...) { + dst1->~Dst1(); + throw; + } +} + /** * The simplest possible map slot. It stores the slot state and the optional key and value * instances in separate variables. Depending on the alignment requirement of the key and value, @@ -83,8 +96,10 @@ template class SimpleMapSlot { { state_ = other.state_; if (other.state_ == Occupied) { - new (&key_buffer_) Key(*other.key_buffer_); - new (&value_buffer_) Value(*other.value_buffer_); + initialize_pointer_pair(other.key_buffer_.ref(), + other.value_buffer_.ref(), + key_buffer_.ptr(), + value_buffer_.ptr()); } } @@ -93,12 +108,15 @@ template class SimpleMapSlot { * from the other have to moved as well. The other slot stays in the state it was in before. Its * optionally stored key and value remain in a moved-from state. */ - SimpleMapSlot(SimpleMapSlot &&other) noexcept + SimpleMapSlot(SimpleMapSlot &&other) noexcept( + std::is_nothrow_move_constructible_v &&std::is_nothrow_move_constructible_v) { state_ = other.state_; if (other.state_ == Occupied) { - new (&key_buffer_) Key(std::move(*other.key_buffer_)); - new (&value_buffer_) Value(std::move(*other.value_buffer_)); + initialize_pointer_pair(std::move(other.key_buffer_.ref()), + std::move(other.value_buffer_.ref()), + key_buffer_.ptr(), + value_buffer_.ptr()); } } @@ -160,21 +178,6 @@ template class SimpleMapSlot { return hash(*key_buffer_); } - /** - * Move the other slot into this slot and destruct it. We do destruction here, because this way - * we can avoid a comparison with the state, since we know the slot is occupied. - */ - void relocate_occupied_here(SimpleMapSlot &other, uint64_t UNUSED(hash)) - { - BLI_assert(!this->is_occupied()); - BLI_assert(other.is_occupied()); - state_ = Occupied; - new (&key_buffer_) Key(std::move(*other.key_buffer_)); - new (&value_buffer_) Value(std::move(*other.value_buffer_)); - other.key_buffer_.ref().~Key(); - other.value_buffer_.ref().~Value(); - } - /** * Returns true, when this slot is occupied and contains a key that compares equal to the given * key. The hash can be used by other slot implementations to determine inequality faster. @@ -196,19 +199,27 @@ template class SimpleMapSlot { void occupy(ForwardKey &&key, ForwardValue &&value, uint64_t hash) { BLI_assert(!this->is_occupied()); - this->occupy_without_value(std::forward(key), hash); new (&value_buffer_) Value(std::forward(value)); + this->occupy_no_value(std::forward(key), hash); + state_ = Occupied; } /** - * Change the state of this slot from empty/removed to occupied, but leave the value - * uninitialized. The caller is responsible to construct the value afterwards. + * Change the state of this slot from empty/removed to occupied. The value is assumed to be + * constructed already. */ - template void occupy_without_value(ForwardKey &&key, uint64_t UNUSED(hash)) + template void occupy_no_value(ForwardKey &&key, uint64_t UNUSED(hash)) { BLI_assert(!this->is_occupied()); + try { + new (&key_buffer_) Key(std::forward(key)); + } + catch (...) { + /* The value is assumed to be constructed already, so it has to be destructed as well. */ + value_buffer_.ref().~Value(); + throw; + } state_ = Occupied; - new (&key_buffer_) Key(std::forward(key)); } /** @@ -218,17 +229,17 @@ template class SimpleMapSlot { void remove() { BLI_assert(this->is_occupied()); - state_ = Removed; key_buffer_.ref().~Key(); value_buffer_.ref().~Value(); + state_ = Removed; } }; /** - * An IntrusiveMapSlot uses two special values of the key to indicate whether the slot is empty or - * removed. This saves some memory in all cases and is more efficient in many cases. The KeyInfo - * type indicates which specific values are used. An example for a KeyInfo implementation is - * PointerKeyInfo. + * An IntrusiveMapSlot uses two special values of the key to indicate whether the slot is empty + * or removed. This saves some memory in all cases and is more efficient in many cases. The + * KeyInfo type indicates which specific values are used. An example for a KeyInfo + * implementation is PointerKeyInfo. * * The special key values are expected to be trivially destructible. */ @@ -297,16 +308,6 @@ template class IntrusiveMapSlot return hash(key_); } - void relocate_occupied_here(IntrusiveMapSlot &other, uint64_t UNUSED(hash)) - { - BLI_assert(!this->is_occupied()); - BLI_assert(other.is_occupied()); - key_ = std::move(other.key_); - new (&value_buffer_) Value(std::move(*other.value_buffer_)); - other.key_.~Key(); - other.value_buffer_.ref().~Value(); - } - template bool contains(const ForwardKey &key, const IsEqual &is_equal, uint64_t UNUSED(hash)) const { @@ -319,22 +320,28 @@ template class IntrusiveMapSlot { BLI_assert(!this->is_occupied()); BLI_assert(KeyInfo::is_not_empty_or_removed(key)); - this->occupy_without_value(std::forward(key), hash); new (&value_buffer_) Value(std::forward(value)); + this->occupy_no_value(std::forward(key), hash); } - template void occupy_without_value(ForwardKey &&key, uint64_t UNUSED(hash)) + template void occupy_no_value(ForwardKey &&key, uint64_t UNUSED(hash)) { BLI_assert(!this->is_occupied()); BLI_assert(KeyInfo::is_not_empty_or_removed(key)); - key_ = std::forward(key); + try { + key_ = std::forward(key); + } + catch (...) { + value_buffer_.ref().~Value(); + throw; + } } void remove() { BLI_assert(this->is_occupied()); - KeyInfo::remove(key_); value_buffer_.ref().~Value(); + KeyInfo::remove(key_); } }; -- cgit v1.2.3