Merge branch 'feature/fix-variant' into 'master'

Fix variant fallback

See merge request !7

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,11 +143,22 @@ 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) {
-      Dune::Hybrid::ifElse(std::is_same<Tp, Head_>(),
-        [&](auto&& id)    { head_=std::move(id(obj)); },
-        [&](auto&& id) { return id(tail_).set(std::move(obj)); }
+    void set(Tp&& obj) {
+      using T = std::decay_t<Tp>;
+      Dune::Hybrid::ifElse(std::is_same<T, Head_>(),
+        [&](auto&& id)    { id(head_)=std::forward<Tp>(obj); },
+        [&](auto&& id) { return id(tail_).set(std::forward<Tp>(obj)); }
       );
     }
 
@@ -137,7 +167,7 @@ namespace Impl {
     }
 
     private:
-    TypeStorage_<Head_, std::is_literal_type<Head_>::value> head_;
+    TypeStorage_<Head_, std::is_trivial<Head_>::value> head_;
     variant_union_<Tail_...> tail_;
   };
 
@@ -145,7 +175,8 @@ namespace Impl {
   struct variant_{
 
     constexpr variant_() :
-      unions_() {}
+      unions_(),
+      index_(invalidIndex) {}
 
     template<typename Tp>
     constexpr variant_(Tp obj) :
@@ -155,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;
@@ -209,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>());
     }
@@ -236,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

dune/subgrid/test/testvariant.cc

@@ -83,6 +83,33 @@ Dune::TestSuite testVariant() {
   suite.check(Std::visit(size, constv2)== 2, "Test const visit");
   suite.check(Std::get_if<V2>(constv2) != nullptr, "Test const get_if");
 
+  /// test copy and move construction/assignment
+  {
+    auto variant_copy_constructed = variant2;
+    // test if deep copy happened
+    auto getPtrToData = [&](const auto& vec) {return static_cast<const void*>(vec.data()); };
+    suite.check(Std::visit(getPtrToData, variant_copy_constructed) != Std::visit(getPtrToData, variant2), "Check deep copy") << "Both vector copies point to same data";
+
+    auto variant_move_constructed = std::move(variant_copy_constructed);
+    // TODO: Add sensible test condition here.
+    // Testing if the pointer to the data is the same as before is probably not a good idea,
+    // as moving does not imply that the data really stays at the same place (though it probably
+    // does).
+    // At least if this compiles and runs we can be confident no double frees happened.
+    //
+    // First idea: Test if the state looks somewhat valid
+    suite.check(Std::holds_alternative<V2>(variant_move_constructed), "Check if move constructed variant holds the right type");
+
+    Std::variant<V, V2> variant_copy_assigned;
+    variant_copy_assigned = variant2;
+    // test if deep copy happened
+    suite.check(Std::visit(getPtrToData, variant_copy_assigned) != Std::visit(getPtrToData, variant2), "Check deep copy at operator=") << "Both vector copies point to same data";
+
+    Std::variant<V, V2> variant_move_assigned;
+    variant_move_assigned = std::move(variant_copy_assigned);
+    // TODO: Again, as above, find a better test for this.
+    suite.check(Std::holds_alternative<V2>(variant_move_assigned), "Check if move assigned variant holds the right type");
+  }
 
   return suite;
 }