From 0f0102ae766d164fcf98adb957eae1580b5a975d Mon Sep 17 00:00:00 2001
From: Sam Yates <halfflat@gmail.com>
Date: Mon, 7 Sep 2020 20:26:38 +0200
Subject: [PATCH] Fix bugs in util::variant assignment implementation. (#1137)
* Fix copy-paste error that led to legitimate copies throwing a bad_variant_access exception.
* Fix exception safety for the case when an error thrown in copy assignment takes the lhs to valueless.
* Fix wrong valueless semantics associated with an exception being thrown during move assignment.
* Add unit tests that exercise these issues.
Fixes #1136.
---
arbor/include/arbor/util/variant.hpp | 14 +++--
test/unit/test_variant.cpp | 80 ++++++++++++++++++++++++++++
2 files changed, 89 insertions(+), 5 deletions(-)
diff --git a/arbor/include/arbor/util/variant.hpp b/arbor/include/arbor/util/variant.hpp
index 22fec8f1..1f3ca0ca 100644
--- a/arbor/include/arbor/util/variant.hpp
+++ b/arbor/include/arbor/util/variant.hpp
@@ -218,7 +218,6 @@ struct variant_dynamic_impl<H, T...> {
else {
variant_dynamic_impl<T...>::assign(i-1, data, from);
}
- if (i!=std::size_t(-1)) throw bad_variant_access{};
}
static void move_assign(std::size_t i, char* data, const char* from) {
@@ -344,10 +343,17 @@ struct variant {
}
}
else {
+ auto old_which = which_;
which_ = npos;
if (x.which_!=npos) {
- variant_dynamic_impl<T...>::assign(x.which_, data, x.data);
- which_ = x.which_;
+ try {
+ variant_dynamic_impl<T...>::assign(x.which_, data, x.data);
+ which_ = x.which_;
+ }
+ catch (...) {
+ variant_dynamic_impl<T...>::destroy(old_which, data);
+ throw;
+ }
}
}
return *this;
@@ -365,10 +371,8 @@ struct variant {
}
}
else {
- which_ = npos;
if (x.which_!=npos) {
variant_dynamic_impl<T...>::move_assign(x.which_, data, x.data);
- which_ = x.which_;
}
}
return *this;
diff --git a/test/unit/test_variant.cpp b/test/unit/test_variant.cpp
index 50708993..8fe0e941 100644
--- a/test/unit/test_variant.cpp
+++ b/test/unit/test_variant.cpp
@@ -270,6 +270,86 @@ TEST(variant, valueless) {
EXPECT_EQ(std::size_t(-1), vi.index());
}
+namespace {
+struct nope {};
+
+struct maybe_throws_on_assign {
+ int i;
+ explicit maybe_throws_on_assign(int i): i(i) {}
+ maybe_throws_on_assign(const maybe_throws_on_assign& x): i(x.i) {}
+ maybe_throws_on_assign(maybe_throws_on_assign&& x): i(x.i) {}
+
+ maybe_throws_on_assign& operator=(const maybe_throws_on_assign& x) {
+ if (x.i<0) throw nope{};
+ i = x.i;
+ return *this;
+ }
+
+ maybe_throws_on_assign& operator=(maybe_throws_on_assign&& x) {
+ if (x.i<0) throw nope{};
+ i = x.i;
+ return *this;
+ }
+
+ ~maybe_throws_on_assign() { ++dtor; }
+
+ static int dtor;
+};
+
+int maybe_throws_on_assign::dtor = 0;
+} // anonymous namespace
+
+TEST(variant, copy_assign) {
+ struct X {
+ X& operator=(const X&) { throw nope{}; }
+ };
+
+ using vidX = variant<int, double, X>;
+
+ vidX v0{in_place_type<int>(), 3};
+ vidX v1{in_place_type<double>(), 4.};
+
+ vidX valueless{X{}};
+ try { valueless = valueless; } catch (...) {}
+ ASSERT_TRUE(valueless.valueless_by_exception());
+
+ vidX v;
+ v = v0;
+ ASSERT_EQ(0u, v.index());
+ EXPECT_EQ(3, get<0>(v));
+
+ v = v1;
+ ASSERT_EQ(1u, v.index());
+ EXPECT_EQ(4., get<1>(v));
+
+ v = valueless;
+ EXPECT_TRUE(v.valueless_by_exception());
+
+ v = v1;
+ ASSERT_EQ(1u, v.index());
+ EXPECT_EQ(4., get<1>(v));
+
+ using vbM = variant<bool, maybe_throws_on_assign>;
+ maybe_throws_on_assign::dtor = 0;
+
+ vbM w0(in_place_type<maybe_throws_on_assign>(), 2);
+ vbM w1(in_place_type<maybe_throws_on_assign>(), -2);
+ ASSERT_EQ(0, maybe_throws_on_assign::dtor);
+
+ EXPECT_THROW(w0 = w1, nope);
+ EXPECT_TRUE(w0.valueless_by_exception());
+ EXPECT_EQ(1, maybe_throws_on_assign::dtor);
+
+ maybe_throws_on_assign::dtor = 0;
+ vbM w2(in_place_type<maybe_throws_on_assign>(), 2);
+
+ ASSERT_EQ(0, maybe_throws_on_assign::dtor);
+
+ EXPECT_THROW(w2 = std::move(w1), nope);
+ EXPECT_FALSE(w2.valueless_by_exception());
+ EXPECT_EQ(0, maybe_throws_on_assign::dtor);
+}
+
TEST(variant, equality) {
struct X {
int i;
--
GitLab