Fix the flat variant implementation

* Add unit tests

include/continuable/detail/flat-variant.hpp

@@ -56,7 +56,7 @@ constexpr std::size_t max_element_of(std::initializer_list<std::size_t> list) {
   return m;
 }
 template <typename... T>
-constexpr std::size_t storage_of_impl() {
+constexpr auto storage_of_impl() {
   constexpr auto size = max_element_of({sizeof(T)...});
   constexpr auto align = max_element_of({alignof(T)...});
   return std::aligned_storage_t<size, align>{};
@@ -101,32 +101,37 @@ struct flat_variant_move_base {
   explicit flat_variant_move_base(flat_variant_move_base&& right) {
     Base& me = *static_cast<Base*>(this);
     Base& other = *static_cast<Base*>(&right);
-    assert(!other.is_empty());
 
-#ifndef _NDEBUG
+    if (other.is_empty()) {
-    me.set_slot(empty_slot::value);
+      me.set_slot(empty_slot::value);
+    } else {
+
+      other.visit([&](auto&& value) {
+#ifndef NDEBUG
+        me.set_slot(empty_slot::value);
 #endif
+        // NOLINTNEXTLINE(misc-move-forwarding-reference)
+        me.init(std::move(value), other.get_slot());
+      });
+    }
 
-    other.visit([&](auto&& value) {
-      // ...
-      me.init(std::move(value));
-    });
-    me.set_slot(other.get());
     other.destroy();
   }
   flat_variant_move_base& operator=(flat_variant_move_base const&) = default;
   flat_variant_move_base& operator=(flat_variant_move_base&& right) {
     Base& me = *static_cast<Base*>(this);
     Base& other = *static_cast<Base*>(&right);
-    assert(!other.is_empty());
 
     me.weak_destroy();
 
-    other.visit([&](auto&& value) {
+    if (other.is_empty()) {
-      // ...
+      me.set_slot(empty_slot::value);
-      me.init(std::move(value));
+    } else {
-    });
+      other.visit([&](auto&& value) {
-    me.set_slot(other.get());
+        // ...
+        me.init(std::move(value), other.get_slot());
+      });
+    }
     other.destroy();
     return *this;
   }
@@ -142,17 +147,17 @@ struct flat_variant_copy_base : flat_variant_move_base<Base> {
   {
     Base& me = *static_cast<Base*>(this);
     Base const& other = *static_cast<Base const*>(&right);
-    assert(!other.is_empty());
 
-#ifndef _NDEBUG
+    if (other.is_empty()) {
-    me.set_slot(empty_slot::value);
+      me.set_slot(empty_slot::value);
+    } else {
+      other.visit([&](auto&& value) {
+#ifndef NDEBUG
+        me.set_slot(empty_slot::value);
 #endif
-
+        me.init(std::move(value), other.get_slot());
-    other.visit([&](auto&& value) {
+      });
-      // ...
+    }
-      me.init(std::move(value));
-    });
-    me.set_slot(other.get());
   }
   flat_variant_copy_base& operator=(flat_variant_copy_base&&) = default;
   flat_variant_copy_base& operator=(flat_variant_copy_base const& right)
@@ -160,15 +165,17 @@ struct flat_variant_copy_base : flat_variant_move_base<Base> {
   {
     Base& me = *static_cast<Base*>(this);
     Base const& other = *static_cast<Base const*>(&right);
-    assert(!other.is_empty());
 
     me.weak_destroy();
 
-    other.visit([&](auto&& value) {
+    if (other.is_empty()) {
-      // ...
+      me.set_slot(empty_slot::value);
-      me.init(std::move(value));
+    } else {
-    });
+      other.visit([&](auto&& value) {
-    me.set_slot(other.get());
+        // ...
+        me.init(std::move(value), other.get_slot());
+      });
+    }
     return *this;
   }
 };
@@ -204,6 +211,8 @@ class flat_variant
           detail::every<std::is_copy_constructible, T...>::value>,
       detail::flat_variant_base<T...> {
 
+  static_assert(sizeof...(T) > 0, "At least one paremeter T is required!");
+
   template <typename...>
   friend class flat_variant;
   template <typename>
@@ -213,9 +222,10 @@ class flat_variant
 
   template <typename V>
   flat_variant(V&& value, detail::slot_t const slot) {
-    using type = std::decay_t<V>;
+#ifndef NDEBUG
-    new (&this->storage_) type(std::forward<V>(value));
+    set_slot(detail::empty_slot::value);
-    set_slot(slot);
+#endif
+    init(std::forward<V>(value), slot);
   }
 
 public:
@@ -232,23 +242,22 @@ public:
 
   template <
       typename V,
-      std::enable_if_t<!is_flat_variant<std::decay_t<V>>::value>* = nullptr,
+      std::enable_if_t<!is_flat_variant<std::decay_t<V>>::value>* = nullptr>
-      std::size_t Index = traits::index_of_t<std::decay_t<V>, T...>::value>
   // Since the flat_variant isn't allowed through SFINAE
   // this overload is safed against the linted issue.
   // NOLINTNEXTLINE(misc-forwarding-reference-overload)
   explicit flat_variant(V&& value)
-      : flat_variant(std::forward<V>(value), Index) {
+      : flat_variant(std::forward<V>(value),
+                     traits::index_of_t<std::decay_t<V>, T...>::value) {
   }
 
   template <
       typename V,
-      std::enable_if_t<!is_flat_variant<std::decay_t<V>>::value>* = nullptr,
+      std::enable_if_t<!is_flat_variant<std::decay_t<V>>::value>* = nullptr>
-      std::size_t Index = traits::index_of_t<std::decay_t<V>, T...>::value>
   flat_variant& operator=(V&& value) {
     weak_destroy();
-    init(std::forward<V>(value));
+    init(std::forward<V>(value),
-    set_slot(Index);
+         traits::index_of_t<std::decay_t<V>, T...>::value);
     return *this;
   }
 
@@ -292,28 +301,27 @@ private:
   void visit(V&& visitor) {
     if (!is_empty()) {
       using callback_t = void (*)(flat_variant*, V &&);
-      constexpr callback_t const callbacks[] = {
+      constexpr callback_t const callbacks[] = {&visit_dispatch<T, V>...};
-          &visit_dispatch<T, V>... // ...
+      callbacks[get_slot()](this, std::forward<V>(visitor));
-      };
-      callbacks[get()](this, std::forward<V>(visitor));
     }
   }
   template <typename V>
   void visit(V&& visitor) const {
     if (!is_empty()) {
       using callback_t = void (*)(flat_variant const*, V&&);
-      constexpr callback_t const callbacks[] = {
+      constexpr callback_t const callbacks[] = {&visit_dispatch_const<T, V>...};
-          &visit_dispatch_const<T, V>... // ...
+      callbacks[get_slot()](this, std::forward<V>(visitor));
-      };
-      callbacks[get()](this, std::forward<V>(visitor));
     }
   }
 
   template <typename V>
-  void init(V&& value) {
+  void init(V&& value, detail::slot_t const slot) {
     assert(is_empty());
-    using type = std::decay_t<decltype(value)>;
+    assert(sizeof(this->storage_) >= sizeof(std::decay_t<V>));
+
+    using type = std::decay_t<V>;
     new (&this->storage_) type(std::forward<V>(value));
+    set_slot(slot);
   }
   void destroy() {
     weak_destroy();
@@ -332,11 +340,11 @@ private:
     set_slot(detail::empty_slot::value);
 #endif
   }
-  detail::slot_t get() const noexcept {
+  detail::slot_t get_slot() const noexcept {
     return this->slot_;
   }
   bool is_slot(detail::slot_t const slot) const noexcept {
-    return get() == slot;
+    return get_slot() == slot;
   }
   void set_slot(detail::slot_t const slot) {
     this->slot_ = slot;

test/unit-test/CMakeLists.txt

@@ -16,6 +16,7 @@ target_link_libraries(test-continuable-base
     continuable-features-noexcept)
 
 add_executable(test-continuable-single
+  ${CMAKE_CURRENT_LIST_DIR}/single/test-continuable-flat-variant.cpp
   ${CMAKE_CURRENT_LIST_DIR}/single/test-continuable-expected.cpp
   ${CMAKE_CURRENT_LIST_DIR}/single/test-continuable-traverse.cpp
   ${CMAKE_CURRENT_LIST_DIR}/single/test-continuable-traverse-async.cpp)

test/unit-test/single/test-continuable-flat-variant.cpp

@@ -0,0 +1,163 @@
+
+/*
+  Copyright(c) 2015 - 2018 Denis Blank <denis.blank at outlook dot com>
+
+  Permission is hereby granted, free of charge, to any person obtaining a copy
+  of this software and associated documentation files(the "Software"), to deal
+  in the Software without restriction, including without limitation the rights
+  to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
+  copies of the Software, and to permit persons to whom the Software is
+  furnished to do so, subject to the following conditions :
+
+  The above copyright notice and this permission notice shall be included in
+  all copies or substantial portions of the Software.
+
+  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE
+  AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+  SOFTWARE.
+**/
+
+#include <memory>
+#include <utility>
+
+#include <continuable/detail/flat-variant.hpp>
+
+#include <test-continuable.hpp>
+
+using cti::detail::container::flat_variant;
+
+static int const CANARY = 373671;
+
+using int_variant = flat_variant<int, tag1, tag2>;
+
+TEST(flat_variant_single_test, is_move_constructible) {
+  {
+    int_variant e_old(CANARY);
+    int_variant e(std::move(e_old));
+
+    EXPECT_TRUE(bool(e));
+    EXPECT_TRUE(e.is<int>());
+    EXPECT_EQ(e.cast<int>(), CANARY);
+  }
+
+  {
+    int_variant e(int_variant(tag1{}));
+
+    EXPECT_TRUE(bool(e));
+    EXPECT_TRUE(e.is<tag1>());
+  }
+}
+
+TEST(flat_variant_single_test, is_value_move_assignable) {
+  int_variant old(CANARY);
+  int_variant e;
+  e = std::move(old);
+
+  EXPECT_TRUE(bool(e));
+  EXPECT_TRUE(e.is<int>());
+  EXPECT_EQ(e.cast<int>(), CANARY);
+}
+
+TEST(flat_variant_single_test, is_copy_constructible) {
+  {
+    int_variant const e_old(CANARY);
+    int_variant e(e_old);
+
+    EXPECT_TRUE(bool(e));
+    EXPECT_TRUE(e.is<int>());
+    EXPECT_EQ(e.cast<int>(), CANARY);
+  }
+
+  {
+    int_variant const e_old(tag1{});
+    int_variant e(e_old);
+
+    EXPECT_TRUE(bool(e));
+    EXPECT_TRUE(e.is<tag1>());
+  }
+}
+
+TEST(flat_variant_single_test, is_copy_assignable) {
+  {
+    int_variant const e_old(CANARY);
+    int_variant e;
+    e = e_old;
+
+    EXPECT_TRUE(bool(e));
+    EXPECT_TRUE(e.is<int>());
+    EXPECT_EQ(e.cast<int>(), CANARY);
+  }
+
+  {
+    int_variant const e_old(tag1{});
+    int_variant e;
+    e = e_old;
+
+    EXPECT_TRUE(bool(e));
+    EXPECT_TRUE(e.is<tag1>());
+  }
+}
+
+// This regression test shows a memory leak which happens when using the
+// expected class move constructed from another expected object.
+TEST(flat_variant_single_test, test_leak_regression) {
+  // expected_all_tests<cti::detail::util::expected<std::__1::unique_ptr<int,
+  //        std::__1::default_delete<int> > > >::supply<int const&>(int const&)
+  //        const
+  //        continuable/build/../test/unit-test/test-continuable-expected.cpp:52
+  // 3:     #3 0x11cf07a in
+  //        expected_all_tests_is_value_assignable_Test<cti::detail::util::expected<std::__1::unique_ptr<int,
+  //        std::__1::default_delete<int> > > >::TestBody()
+  //        continuable/build/../test/unit-test/test-continuable-expected.cpp:133:15
+  // 3:     #4 0x1339e4e in void
+  //        testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,
+  //        void>(testing::Test*, void (testing::Test::*)(), char const*)
+  //        continuable/build/../dep/googletest/googletest/googletest/src/gtest.cc:2395:10
+  using type = std::shared_ptr<int>;
+
+  auto const validate = [](auto&& variant, std::size_t use) {
+    ASSERT_TRUE(variant.template is<type>());
+    ASSERT_EQ((*variant.template cast<type>()), CANARY);
+    ASSERT_EQ(variant.template cast<type>().use_count(), use);
+  };
+
+  bool destroyed = false;
+  {
+    type ptr(new int(CANARY), [&](int* val) {
+      destroyed = true;
+      delete val;
+    });
+
+    auto e1(flat_variant<type>(std::move(ptr)));
+    validate(e1, 1);
+
+    auto e2 = std::move(e1);
+    validate(e2, 1);
+
+    flat_variant<type> e3;
+    e3 = std::move(e2);
+    validate(e3, 1);
+
+    {
+      flat_variant<type> ec(e3);
+      validate(ec, 2);
+    }
+
+    {
+      flat_variant<type> ec = e3;
+      validate(ec, 2);
+    }
+
+    {
+      flat_variant<type> ec;
+      ec = e3;
+      validate(ec, 2);
+    }
+  }
+
+  ASSERT_TRUE(destroyed);
+}