Noexcept on constructor

[joto]

The constructor for variant is declared noexcept which is only true if the constructor for the underlying type is noexcept. Because we don't know which type that will be we can only mark the constructor noexcept if all underlying types have a noexcept constructor.

variant/variant.hpp

Lines 584 to 590 in 861faa8

	VARIANT_INLINE variant(T && val) noexcept
	: type_index(detail::value_traits<typename std::remove_reference<T>::type, Types...>::index)
	{
	constexpr std::size_t index = sizeof...(Types) - detail::value_traits<typename std::remove_reference<T>::type, Types...>::index - 1;
	using target_type = typename std::tuple_element<index, std::tuple<Types...>>::type;
	new (&data) target_type(std::forward<T>(val)); // nothrow
	}

There is a comment // nothrow in there. It is unclear what it means. Maybe it was meant to say that we expect the constructor to be "nothrow", but that really must be encoded in code.

[lightmare]

edit 2: amended for forwarding, is it correct now?

If has_type</*remove_reference?*/ T, Types...> then:

noexcept(std::is_nothrow_constructible<
  typename std::remove_reference<T>::type,
  T&&
>::value)

If it needs conversion, then perhaps be something like:

noexcept(std::is_nothrow_constructible<
  typename detail::convertible_type_noref<T>::type,
  T&&
>::value)

To reduce clutter I just made up detail::convertible_type_noref<T>, which is:

template <typename T>
using convertible_type_noref<T> = convertible_type<typename std::remove_reference<T>::type>;

and added member ::type which should be obvious what it is and that it could be there ;)

[joto]

What gets constructed is the target_type, not T. Usually this will be the same, but it doesn't have to be if there is implicit conversion going on.

[lightmare]

That's what I said, if has_type<T, Types...> then target_type==T else target_type==convertible_type_noref<T>::type

[joto]

@lightmare not sure I understand what you mean here. Maybe you can create a real pull request so we can better see what you mean.

[lightmare]

Ok, but I'll probably need some help understanding where std::remove_reference should be applied and where not.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0086r0.pdf

variant<int&>
References are supported as alternative types. Assignment to
such a value is ill-formed.

In anthonyw and tomilov implementations, which use an actual union for storage, it's clear what that translates to. But in mapbox::variant using raw byte array and reinterpret_cast, it's difficult to grasp. I think currently it just drops the reference and stores an int.

[joto]

@lightmare: We very likely will not support references at all. They have not worked in the past and it is unclear to me, why there are a few places in the code using std::remove_reference. I have added #70 to fix this.

Looking further, I do think we want proper noexcepts at some point, but this needs a lot more tests to make sure every combination of constructors, assignments, swaps, etc. is handled correctly. Extra problem here is that a wrong noexcept declaration will not be detected by the compiler.

I think the way forward is to first remove noexcept everywhere. We should only keep it on (or add it to) functions where it is a) obviously correct and b) any implementation change we might do in the future will preserve it. This way we get to a correct interface fast. Adding more noexcepts later is then backwards compatible and can be done piece by piece. The drawback is that this is an interface change so it might break something in users of variant. On the other hand the wrong noexcept were always wrong anyway and nobody noticed.

[lightmare]

We should only keep it on (or add it to) functions where it is a) obviously correct and b) any implementation change we might do in the future will preserve it.

I'll add notes to specific lines in my previous pull req. indicating which are a), b) or neither.

[lightmare]

Notes added to lightmare@244c3b7