Merge pull request #109 from iris-cpp/fix-rvariant

Fix some bugs on rvariant for type-changing case

Author: saki7

include/iris/rvariant/rvariant.hpp

@@ -452,22 +452,15 @@ IRIS_RVARIANT_ALWAYS_THROWING_UNREACHABLE_BEGIN
             this->template reset_construct_never_valueless<I>(std::forward<Args>(args)...);
 
         } else {
-            this->raw_visit([&, this]<std::size_t old_i, class T_old_i>(std::in_place_index_t<old_i>, T_old_i& t_old_i)
-                noexcept(std::is_nothrow_constructible_v<T, Args...>)
-            {
+            this->raw_visit([&, this]<std::size_t old_i, class T_old_i>(std::in_place_index_t<old_i>, T_old_i& t_old_i) {
                 static_assert(!std::is_reference_v<T_old_i>);
                 static_assert(!std::is_const_v<T_old_i>);
 
                 if constexpr (old_i == std::variant_npos) {
                     (void)t_old_i;
                     this->template construct_on_valueless<I>(std::forward<Args>(args)...);
 
-                } else if constexpr (std::is_nothrow_constructible_v<T, Args...>) {
-                    t_old_i.~T_old_i();
-                    static_assert(std::is_nothrow_constructible_v<storage_type, std::in_place_index_t<old_i>, Args...>);
-                    std::construct_at(&this->storage_, std::in_place_index<old_i>, std::forward<Args>(args)...);
-
-                } else if constexpr (std::is_same_v<T_old_i, T>) { // NOT type-changing
+                } else if constexpr (old_i == I) { // same alternative
                     if constexpr (
                         (sizeof(T) <= detail::never_valueless_trivial_size_limit && std::is_trivially_move_assignable_v<T>) ||
                         is_recursive_wrapper_v<T>
@@ -497,12 +490,12 @@ IRIS_RVARIANT_ALWAYS_THROWING_UNREACHABLE_BEGIN
                         static_assert(!never_valueless);
                         t_old_i.~T_old_i();
                         this->index_ = detail::variant_npos<sizeof...(Ts)>;
-                        static_assert(!noexcept(std::construct_at(&this->storage(), std::in_place_index<old_i>, std::forward<Args>(args)...)));
-                        std::construct_at(&this->storage_, std::in_place_index<old_i>, std::forward<Args>(args)...); // may throw
-                        this->index_ = old_i;
+                        static_assert(!noexcept(std::construct_at(&this->storage(), std::in_place_index<I>, std::forward<Args>(args)...)));
+                        std::construct_at(&this->storage_, std::in_place_index<I>, std::forward<Args>(args)...); // may throw
+                        this->index_ = I;
                     }
 
-                } else { // type-changing
+                } else { // another alternative, possibly of the same type
                     if constexpr (
                         (sizeof(T) <= detail::never_valueless_trivial_size_limit && std::is_trivially_move_constructible_v<T>) ||
                         is_recursive_wrapper_v<T>

test/rvariant/rvariant.cpp

@@ -14,6 +14,7 @@
 #include <exception>
 #include <initializer_list>
 #include <memory>
+#include <string>
 #include <type_traits>
 #include <utility>
 #include <variant>
@@ -1422,6 +1423,64 @@ TEST_CASE("emplace")
     STATIC_REQUIRE(is_never_valueless<iris::rvariant<iris::recursive_wrapper<int>>>);
 
     // ReSharper restore CppStaticAssertFailure
+
+    // `emplace<I>` must switch to the I-th alternative even when the current one has the same type
+    {
+        // NOLINTBEGIN(modernize-use-equals-default)
+        {
+            struct S
+            {
+                S(int value) noexcept(false) : value(value) {} // potentially-throwing
+                int value = 0;
+            };
+            iris::rvariant<S, S> v(std::in_place_index<0>, 1);
+            v.emplace<1>(2);
+            REQUIRE(v.index() == 1);
+            CHECK(iris::get<1>(v).value == 2);
+            v.emplace<0>(3);
+            REQUIRE(v.index() == 0);
+            CHECK(iris::get<0>(v).value == 3);
+        }
+        {
+            struct StrangeS
+            {
+                StrangeS(int value) noexcept(false) : value(value) {} // potentially-throwing
+                StrangeS(StrangeS&& other) noexcept : value(other.value) {} // not trivial
+                StrangeS(StrangeS const&) = default; // trivial
+                StrangeS& operator=(StrangeS&& other) noexcept { value = other.value; return *this; } // not trivial
+                StrangeS& operator=(StrangeS const&) = default; // trivial
+                int value = 0;
+            };
+            iris::rvariant<StrangeS, StrangeS> v(std::in_place_index<0>, 1);
+            v.emplace<1>(2);
+            REQUIRE(v.index() == 1);
+            CHECK(iris::get<1>(v).value == 2);
+            v.emplace<0>(3);
+            REQUIRE(v.index() == 0);
+            CHECK(iris::get<0>(v).value == 3);
+        }
+        // NOLINTEND(modernize-use-equals-default)
+        {
+            iris::rvariant<std::string, std::string> v(std::in_place_index<0>, "a");
+            v.emplace<1>("b");
+            REQUIRE(v.index() == 1);
+            CHECK(iris::get<1>(v) == "b");
+            v.emplace<0>("c");
+            REQUIRE(v.index() == 0);
+            CHECK(iris::get<0>(v) == "c");
+        }
+        {
+            iris::rvariant<iris::recursive_wrapper<int>, iris::recursive_wrapper<int>> v(std::in_place_index<0>, 1);
+            v.emplace<1>(2);
+            REQUIRE(v.index() == 1);
+            CHECK(iris::get<1>(v) == 2);
+        }
+        STATIC_CHECK([] {
+            iris::rvariant<std::string, std::string> v(std::in_place_index<0>, "a");
+            v.emplace<1>("b");
+            return v.index() == 1 && iris::get<1>(v) == "b";
+        }());
+    }
 }