Merge pull request #16911 from anton-potapov:ap/variant_move_issue

modules/gapi/include/opencv2/gapi/util/variant.hpp

@@ -36,11 +36,14 @@ namespace util
             static const constexpr std::size_t value = I;
         };
 
-
-        template<class T, class U, class V> using are_different =
-            std::enable_if<!std::is_same<typename std::decay<T>::type,
-                                         typename std::decay<U>::type>::value,
-                           V>;
+        template< bool B, class T = void >
+        using enable_if_t = typename std::enable_if<B,T>::type;
+
+        template<class T, class U, class V = void>
+        using are_different_t = enable_if_t<
+                !std::is_same<typename std::decay<T>::type,
+                              typename std::decay<U>::type>::value,
+                 V>;
     }
 
     template<typename Target, typename... Types>
@@ -157,10 +160,17 @@ namespace util
         variant(const variant& other);
         variant(variant&& other) noexcept;
         template<typename T> explicit variant(const T& t);
-        // are_different is a SFINAE trick to avoid variant(T &&t) with T=variant
+        // are_different_t is a SFINAE trick to avoid variant(T &&t) with T=variant
         // for some reason, this version is called instead of variant(variant&& o) when
-        // variant is used in STL containers (examples: vector assignment)
-        template<typename T> explicit variant(T&& t, typename detail::are_different<variant, T, int>::type = 0);
+        // variant is used in STL containers (examples: vector assignment).
+        // detail::enable_if_t<! std::is_lvalue_reference<T>::value> is a SFINAE
+        // trick to limit this constructor only to rvalue reference argument
+        template<
+            typename T,
+            typename = detail::are_different_t<variant, T>,
+            typename = detail::enable_if_t<! std::is_lvalue_reference<T>::value>
+        >
+        explicit variant(T&& t);
         // template<class T, class... Args> explicit variant(Args&&... args);
         // FIXME: other constructors
 
@@ -172,9 +182,11 @@ namespace util
         variant& operator=(variant &&rhs) noexcept;
 
         // SFINAE trick to avoid operator=(T&&) with T=variant<>, see comment above
-        template<class T>
-        typename detail::are_different<variant, T, variant&>
-        ::type operator=(T&& t) noexcept;
+        template<
+            typename T,
+            typename = detail::are_different_t<variant, T>
+        >
+        variant& operator=(T&& t) noexcept;
 
         // Observers
         std::size_t index() const noexcept;
@@ -232,8 +244,8 @@ namespace util
     }
 
     template<typename... Ts>
-    template<class T>
-    variant<Ts...>::variant(T&& t, typename detail::are_different<variant, T, int>::type)
+    template<class T, typename , typename>
+    variant<Ts...>::variant(T&& t)
         : m_index(util::type_list_index<typename std::remove_reference<T>::type, Ts...>::value)
     {
         (mctrs()[m_index])(memory, &t);
@@ -278,8 +290,8 @@ namespace util
     }
 
     template<typename... Ts>
-    template<class T> typename detail::are_different<variant<Ts...>, T, variant<Ts...>&>
-    ::type variant<Ts...>::operator=(T&& t) noexcept
+    template<typename T, typename>
+    variant<Ts...>& variant<Ts...>::operator=(T&& t) noexcept
     {
         using decayed_t = typename std::decay<T>::type;
         // FIXME: No version with implicit type conversion available!
@@ -288,10 +300,10 @@ namespace util
 
         if (t_index == m_index)
         {
-            util::get<decayed_t>(*this) = std::move(t);
+            util::get<decayed_t>(*this) = std::forward<T>(t);
             return *this;
         }
-        else return (*this = variant(std::move(t)));
+        else return (*this = variant(std::forward<T>(t)));
     }
 
     template<typename... Ts>

modules/gapi/test/util/variant_tests.cpp

@@ -33,7 +33,7 @@ TEST(Variant, EmptyCTor)
     EXPECT_EQ("", util::get<std::string>(vsi));
 }
 
-TEST(Variant, ValueMoveCTor)
+TEST(Variant, ConvertingCTorMove)
 {
     util::variant<int> vi(42);
     EXPECT_EQ(0u,     vi.index());
@@ -55,16 +55,23 @@ TEST(Variant, ValueMoveCTor)
     EXPECT_EQ(0u,     vsi.index());
     EXPECT_EQ("2017", util::get<std::string>(vsi));
 
+    std::string rvs("2017");
+    util::variant<std::string, int> vsi3(std::move(rvs));
+    EXPECT_EQ(0u,     vsi3.index());
+    EXPECT_EQ("2017", util::get<std::string>(vsi3));
+    EXPECT_EQ("", rvs) <<"Rvalue source argument was not moved from while should?";
+
     util::variant<std::string, int> vsi2(42);
     EXPECT_EQ(1u,     vsi2.index());
     EXPECT_EQ(42,     util::get<int>(vsi2));
 }
 
-TEST(Variant, ValueCopyCTor)
+TEST(Variant, ConvertingCTorCopy)
 {
     const int i42         = 42;
     const int i17         = 2017;
     const std::string s17 = "2017";
+    std::string s17_lvref = s17;
 
     util::variant<int> vi(i42);
     EXPECT_EQ(0u,     vi.index());
@@ -82,6 +89,11 @@ TEST(Variant, ValueCopyCTor)
     EXPECT_EQ(0u,     vs.index());
     EXPECT_EQ(s17,    util::get<std::string>(vs));
 
+    util::variant<std::string> vs_lv(s17_lvref);
+    EXPECT_EQ(0u,           vs_lv.index());
+    EXPECT_EQ(s17, s17_lvref);
+    EXPECT_EQ(s17_lvref,    util::get<std::string>(vs_lv));
+
     util::variant<std::string, int> vsi(s17);
     EXPECT_EQ(0u,     vsi.index());
     EXPECT_EQ(s17, util::get<std::string>(vsi));
@@ -151,6 +163,20 @@ TEST(Variant, Assign_ValueUpdate_DiffType)
     EXPECT_EQ("42", util::get<std::string>(vis));
 }
 
+TEST(Variant, Assign_RValueRef_DiffType)
+{
+    TestVar vis(42);
+
+    EXPECT_EQ(0u, vis.index());
+    EXPECT_EQ(42, util::get<int>(vis));
+
+    std::string s("42");
+    vis = std::move(s);
+    EXPECT_EQ(1u, vis.index());
+    EXPECT_EQ("42", util::get<std::string>(vis));
+    EXPECT_EQ("", s) << "right hand side argument of assignment operation was not moved from while should?";;
+}
+
 TEST(Variant, Assign_LValueRef_DiffType)
 {
     TestVar vis(42);
@@ -162,6 +188,7 @@ TEST(Variant, Assign_LValueRef_DiffType)
     vis = s;
     EXPECT_EQ(1u, vis.index());
     EXPECT_EQ("42", util::get<std::string>(vis));
+    EXPECT_EQ("42", s) << "right hand side argument of assignment operation was moved from while should not ?";
 }
 
 TEST(Variant, Assign_ValueUpdate_Const)
@@ -198,7 +225,7 @@ TEST(Variant, Assign_ValueUpdate_Const_DiffType)
     EXPECT_EQ("42", util::get<std::string>(va));
 }
 
-TEST(Variant, Assign_Move)
+TEST(Variant, Assign_Move_Variant)
 {
     TestVar va(42);
     TestVar vb(std::string("42"));