mapbox / variant

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

Noexcept on operator=() is wrong #88

Open joto opened 8 years ago

joto commented 8 years ago

I think the noexcept on operator() here is wrong. There are several operations involved which all might fail.

lightmare commented 8 years ago

Yes, should be

noexcept(noexcept(variant<Types...>(std::forward<T>(rhs))) &&
         std::is_move_constructible<std::tuple<Types...>>::value)

The first part covers is_constructible<V, T&&> && is_destructible<V>.

Edit: assignable->constructible, as helper::move calls constructor.

It'd probably be best to dispatch on rvalue-ness of T, so that it doesn't have to make two moves if T is an rvalue, but that can be left to a later improvement.

While you're looking at it... the copy-assignment below makes an unnecessary second copy; and it is actually redundant -- the universal covers lvalue references as well.

joto commented 8 years ago

I don't feel comfortable having such complex code (especially in areas with compiler problems like #86) without any tests. And it is really easy to get the implementation of the function and the noexcept() clause out of sync. How are we going to test this?

lightmare commented 8 years ago

In that case, just remove both conversion-assignment operators, they're not needed -- actually the compiler will generate exactly the code in the first, and better than the second:

v = x; // conversion constructor from decltype(x) must be available
// the compiler will do
v = V(x); // i.e. construct a temporary variant and move-assign =(V&&)