mapbox / variant

C++11/C++14 Variant
BSD 3-Clause "New" or "Revised" License
371 stars 101 forks source link

Fix the noexcept specifications for move assignment and conversion. #160

Closed mlogan closed 6 years ago

mlogan commented 7 years ago

This should allow variants to be nothrow move-assignable where appropriate.

I also took a crack at fixing the noexcept spec on the conversion assignment operator, which could previously lie. I definitely don't understand the meta-programming used here, so comments are welcome.

lightmare commented 7 years ago

Move assignment fix looks good.

As for conversion assignment, both operators are redundant and should've been removed long ago.

mlogan commented 7 years ago

Want me to revert the conversion assignment changes? I tried removing them but it breaks the tests. I'm not sure why, but variant<T...>(variant<T...>&&) and operator=(variant<T...>&&) aren't chosen even when given a type in T....

lightmare commented 7 years ago

Well I don't have the authority to accept/decline, I meant that as a hint.

The two conversion operators are superfluous, instead of fixing their declarations just remove them. I checked out this PR (refs/pull/160/merge), deleted them and tests pass for me (tried gcc 5.4/6.2 and clang 3.9/4.0):

diff --git a/include/mapbox/variant.hpp b/include/mapbox/variant.hpp
index a6b7c8b..c8e2b76 100644
--- a/include/mapbox/variant.hpp
+++ b/include/mapbox/variant.hpp
@@ -661,30 +661,6 @@ public:
         return *this;
     }

-    // conversions
-    // move-assign
-    template <typename T, typename Traits = detail::value_traits<T, Types...>,
-              typename Enable = typename std::enable_if<Traits::is_valid && !std::is_same<variant<Types...>, typename Traits::value_type>::value>::type >
-    VARIANT_INLINE variant<Types...>& operator=(T&& rhs)
-        // not that we check is_nothrow_constructible<T>, not is_nothrow_move_assignable<T>,
-        // since we construct a temporary
-        noexcept(std::is_nothrow_constructible<typename Traits::target_type, T&&>::value
-                 && std::is_nothrow_move_assignable<variant<Types...>>::value)
-    {
-        variant<Types...> temp(std::forward<T>(rhs));
-        move_assign(std::move(temp));
-        return *this;
-    }
-
-    // copy-assign
-    template <typename T>
-    VARIANT_INLINE variant<Types...>& operator=(T const& rhs)
-    {
-        variant<Types...> temp(rhs);
-        copy_assign(temp);
-        return *this;
-    }
-
     template <typename T, typename std::enable_if<
                           (detail::direct_type<T, Types...>::index != detail::invalid_value)>::type* = nullptr>
     VARIANT_INLINE bool is() const
mlogan commented 7 years ago

Ahhh - I thought you were suggesting to also remove the ctor variant<T...>(T&&). Removing that definitely breaks things.

I also tried removing only the conversions, and while that breaks no existing tests, it breaks one of the new tests that I added.

mlogan commented 6 years ago

@artemp Any interest in getting this PR merged?

artemp commented 6 years ago

@mlogan - looks good, thanks!

mlogan commented 6 years ago

Thanks!