From 5a16caa7eaf55d53937d69615cdcb38a5fc5a5e9 Mon Sep 17 00:00:00 2001
From: Lasse Hinrichsen <lh1887@mi.fu-berlin.de>
Date: Thu, 5 Apr 2018 16:44:20 +0200
Subject: [PATCH] [variant] implement Rule of Five
If the variant object was destroyed, the memory for the object itself was properly
free'd. However, the dtor would never been called on the object (unless it's of literal type)
and hence memory that was itself managed by the object would leak. The same reasoning
applies if the type of the object held by the variant changed. This commit fixes this and cleans up
when appropiate.
---
dune/subgrid/common/variant.hh | 89 ++++++++++++++++++++++++++++++++--
1 file changed, 84 insertions(+), 5 deletions(-)
diff --git a/dune/subgrid/common/variant.hh b/dune/subgrid/common/variant.hh
index 85eb8bf..4b23e38 100644
--- a/dune/subgrid/common/variant.hh
+++ b/dune/subgrid/common/variant.hh
@@ -11,6 +11,9 @@ namespace Dune {
namespace Std {
namespace Impl {
+ // indicator value if something's not yet (or not any longer) valid
+ constexpr const auto invalidIndex = std::numeric_limits<size_t>::max();
+
/* helper constructs to find position of a type T in a pack Ts... */
template <typename T, typename... Ts>
struct index_in_pack;
@@ -63,6 +66,9 @@ namespace Impl {
const auto& get() const {
return tp_;
}
+
+ void reset() {};
+
private:
Tp tp_;
};
@@ -78,6 +84,8 @@ namespace Impl {
::new (&tp_) Tp(std::forward<Args>(args)...);
}
+ TypeStorage_() = delete;
+
auto& get() {
return *(tp_.ptr());
}
@@ -85,12 +93,23 @@ namespace Impl {
return *(tp_.ptr());
}
+ void reset() {
+ // Properly destruct the member:
+ tp_.ptr()->~Tp();
+ // (the memory itself doesn't need to be free'd. This is done when the Buffer_ member gets destructed)
+ }
+
private:
Buffer_<Tp> tp_;
};
template<typename... T>
- union variant_union_ {};
+ union variant_union_ {
+ // dummy (this should never be called)
+ void resetByIndex(size_t) {
+ assert(false);
+ };
+ };
template<typename Head_, typename... Tail_>
union variant_union_<Head_, Tail_...> {
@@ -124,6 +143,16 @@ namespace Impl {
return tail_.getByIndex(std::integral_constant<size_t, N-1>());
}
+ void resetByIndex(size_t indexToReset) {
+ if (indexToReset == 0) {
+ head_.reset();
+ return;
+ }
+ else {
+ tail_.resetByIndex(indexToReset-1);
+ }
+ }
+
template<typename Tp>
void set(Tp&& obj) {
using T = std::decay_t<Tp>;
@@ -146,7 +175,8 @@ namespace Impl {
struct variant_{
constexpr variant_() :
- unions_() {}
+ unions_(),
+ index_(invalidIndex) {}
template<typename Tp>
constexpr variant_(Tp obj) :
@@ -156,6 +186,45 @@ namespace Impl {
unions_.set(std::move(obj));
}
+ variant_(variant_&& other) {
+ unions_ = std::move(other.unions_);
+ index_ = other.index_;
+ other.index_ = invalidIndex;
+ }
+
+ variant_(const variant_& other) {
+ index_ = other.index_;
+
+ namespace H = Dune::Hybrid;
+ H::forEach(H::integralRange(std::integral_constant<size_t, size_>()), [&](auto i) {
+ if(i==index_)
+ unions_.set(other.get<i>());
+ });
+ }
+
+ variant_& operator=(const variant_& other) {
+ if(index_ != invalidIndex)
+ unions_.resetByIndex(index_);
+
+ index_ = other.index_;
+
+ namespace H = Dune::Hybrid;
+ H::forEach(H::integralRange(std::integral_constant<size_t, size_>()), [&](auto i) {
+ if(i==index_)
+ unions_.set(other.get<i>());
+ });
+ return *this;
+ }
+
+ variant_& operator=(variant_&& other) {
+ unions_ = std::move(other.unions_);
+ index_ = other.index_;
+ other.index_ = invalidIndex;
+
+ return *this;
+ }
+
+
template<typename Tp>
auto& get() {
constexpr size_t idx = index_in_pack<Tp, T...>::value;
@@ -210,21 +279,25 @@ namespace Impl {
template<size_t N>
auto& get() {
- if (index_ != N)
+ if (index_ != N || index_ == invalidIndex)
DUNE_THROW(Dune::Exception, "Bad variant access.");
return unions_.template getByIndex(std::integral_constant<size_t, N>());
}
template<size_t N>
const auto& get() const {
- if (index_ != N)
+ if (index_ != N || index_ == invalidIndex)
DUNE_THROW(Dune::Exception, "Bad variant access.");
return unions_.template getByIndex(std::integral_constant<size_t, N>());
}
template<typename Tp>
constexpr Tp& operator=(Tp obj) {
- unions_.set(std::move(obj));
constexpr auto index = index_in_pack<Tp, T...>::value;
+ // before setting a new object into the buffer, we have to destruct the old element
+ if(not (index_ == index or index_ == invalidIndex)) {
+ unions_.resetByIndex(index_);
+ }
+ unions_.set(std::move(obj));
index_=index;
return unions_.getByIndex(std::integral_constant<size_t,index>());
}
@@ -237,6 +310,12 @@ namespace Impl {
return sizeof...(T);
}
+ ~variant_() {
+ if (index_ != invalidIndex) {
+ unions_.resetByIndex(index_);
+ }
+ }
+
/* \brief Apply visitor to the active variant.
*
* visit assumes that the result of
--
GitLab