Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Noexcept on operator=() is wrong #88

Open
joto opened this issue Feb 1, 2016 · 3 comments
Open

Noexcept on operator=() is wrong #88

joto opened this issue Feb 1, 2016 · 3 comments

Comments

@joto
Copy link
Contributor

joto commented Feb 1, 2016

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

@lightmare
Copy link
Contributor

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
Copy link
Contributor Author

joto commented Feb 1, 2016

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
Copy link
Contributor

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&&)

lightmare added a commit to lightmare/variant that referenced this issue Feb 3, 2016
- they are no better than what the compiler will do to perform such
  assigments without explicit operators - construct a temporary variant
  using conversion constructor, and move-assign from the temporary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants