Fix std::variant regression caused by never-valueless optimization
A regression was introduced by the recent changes to provide the strong exception safety guarantee for "never valueless" types that have O(1), non-throwing move assignment. The problematic code is: else if constexpr (__detail::__variant::_Never_valueless_alt<type>()) { // This construction might throw: variant __tmp(in_place_index<_Np>, __il, std::forward<_Args>(__args)...); // But _Never_valueless_alt<type> means this won't: *this = std::move(__tmp); } When the variant is not assignable, the assignment is ill-formed, so should not be attempted. When the variant has a copy assignment operator but not a move assignment operator, the assignment performs a copy assignment and that could throw, so should not be attempted. The solution is to only take that branch when the variant has a move assignment operator, which is determined by the _Traits::_S_move_assign constant. When that is false the strong exception safety guarantee is not possible, and so the __never_valueless function should also depend on _S_move_assign. While testing the fixes for this I noticed that the partial specialization _Never_valueless_alt<basic_string<C,T,A>> incorrectly assumed that is_nothrow_move_constructible<basic_string<C,T,A>> is always true, but that's wrong for fully-dynamic COW strings. Fix the partial specialization, and improve the comment describing _Never_valueless_alt to be clear it depends on move construction as well as move assignment. Finally, I also observed that _Variant_storage<false, T...>::_M_valid() was not taking advantage of the __never_valueless<T...>() function to avoid a runtime check. Only the _Variant_storage<true, T...>::_M_valid() function was using __never_valueless. That is also fixed. PR libstdc++/87431 * include/bits/basic_string.h (_Never_valueless_alt): Make partial specialization also depend on is_nothrow_move_constructible. * include/std/variant (__detail::__variant::__never_valueless()): Only true if the variant would have a move assignment operator. (__detail::__variant::_Variant_storage<false, T...>::_M_valid()): Check __never_valueless<T...>(). (variant::emplace): Only perform non-throwing move assignments for never-valueless alternatives if the variant has a move assignment operator. * testsuite/20_util/variant/compile.cc: Check that never-valueless types can be emplaced into non-assignable variants. * testsuite/20_util/variant/run.cc: Check that never-valueless types don't get copied when emplaced into non-assignable variants. From-SVN: r270502
This commit is contained in:
parent
be46043e07
commit
47a468bdbe
|
@ -1,5 +1,20 @@
|
|||
2019-04-23 Jonathan Wakely <jwakely@redhat.com>
|
||||
|
||||
PR libstdc++/87431
|
||||
* include/bits/basic_string.h (_Never_valueless_alt): Make partial
|
||||
specialization also depend on is_nothrow_move_constructible.
|
||||
* include/std/variant (__detail::__variant::__never_valueless()):
|
||||
Only true if the variant would have a move assignment operator.
|
||||
(__detail::__variant::_Variant_storage<false, T...>::_M_valid()):
|
||||
Check __never_valueless<T...>().
|
||||
(variant::emplace): Only perform non-throwing move assignments
|
||||
for never-valueless alternatives if the variant has a move assignment
|
||||
operator.
|
||||
* testsuite/20_util/variant/compile.cc: Check that never-valueless
|
||||
types can be emplaced into non-assignable variants.
|
||||
* testsuite/20_util/variant/run.cc: Check that never-valueless types
|
||||
don't get copied when emplaced into non-assignable variants.
|
||||
|
||||
* include/std/variant (__detail::__variant::__ref_cast): Remove
|
||||
unused function.
|
||||
(__detail::__variant::_Uninitialized::_M_get)
|
||||
|
|
|
@ -6849,10 +6849,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
|
|||
template<typename> struct _Never_valueless_alt; // see <variant>
|
||||
|
||||
// Provide the strong exception-safety guarantee when emplacing a
|
||||
// basic_string into a variant, but only if move assignment cannot throw.
|
||||
// basic_string into a variant, but only if moving the string cannot throw.
|
||||
template<typename _Tp, typename _Traits, typename _Alloc>
|
||||
struct _Never_valueless_alt<std::basic_string<_Tp, _Traits, _Alloc>>
|
||||
: std::is_nothrow_move_assignable<std::basic_string<_Tp, _Traits, _Alloc>>
|
||||
: __and_<
|
||||
is_nothrow_move_constructible<std::basic_string<_Tp, _Traits, _Alloc>>,
|
||||
is_nothrow_move_assignable<std::basic_string<_Tp, _Traits, _Alloc>>
|
||||
>::type
|
||||
{ };
|
||||
} // namespace __detail::__variant
|
||||
#endif // C++17
|
||||
|
|
|
@ -340,9 +340,9 @@ namespace __variant
|
|||
{ };
|
||||
|
||||
// Specialize _Never_valueless_alt for other types which have a
|
||||
// non-throwing and cheap move assignment operator, so that emplacing
|
||||
// the type will provide the strong exception-safety guarantee,
|
||||
// by creating and moving a temporary.
|
||||
// non-throwing and cheap move construction and move assignment operator,
|
||||
// so that emplacing the type will provide the strong exception-safety
|
||||
// guarantee, by creating and moving a temporary.
|
||||
// Whether _Never_valueless_alt<T> is true or not affects the ABI of a
|
||||
// variant using that alternative, so we can't change the value later!
|
||||
|
||||
|
@ -352,7 +352,8 @@ namespace __variant
|
|||
template <typename... _Types>
|
||||
constexpr bool __never_valueless()
|
||||
{
|
||||
return (_Never_valueless_alt<_Types>::value && ...);
|
||||
return _Traits<_Types...>::_S_move_assign
|
||||
&& (_Never_valueless_alt<_Types>::value && ...);
|
||||
}
|
||||
|
||||
// Defines index and the dtor, possibly trivial.
|
||||
|
@ -407,6 +408,8 @@ namespace __variant
|
|||
constexpr bool
|
||||
_M_valid() const noexcept
|
||||
{
|
||||
if constexpr (__never_valueless<_Types...>())
|
||||
return true;
|
||||
return this->_M_index != __index_type(variant_npos);
|
||||
}
|
||||
|
||||
|
@ -1428,7 +1431,8 @@ namespace __variant
|
|||
this->_M_reset();
|
||||
__variant_construct_by_index<_Np>(*this, __tmp);
|
||||
}
|
||||
else if constexpr (__detail::__variant::_Never_valueless_alt<type>())
|
||||
else if constexpr (__detail::__variant::_Never_valueless_alt<type>()
|
||||
&& _Traits::_S_move_assign)
|
||||
{
|
||||
// This construction might throw:
|
||||
variant __tmp(in_place_index<_Np>,
|
||||
|
@ -1474,7 +1478,8 @@ namespace __variant
|
|||
__variant_construct_by_index<_Np>(*this, __il,
|
||||
std::forward<_Args>(__args)...);
|
||||
}
|
||||
else if constexpr (__detail::__variant::_Never_valueless_alt<type>())
|
||||
else if constexpr (__detail::__variant::_Never_valueless_alt<type>()
|
||||
&& _Traits::_S_move_assign)
|
||||
{
|
||||
// This construction might throw:
|
||||
variant __tmp(in_place_index<_Np>, __il,
|
||||
|
|
|
@ -479,6 +479,20 @@ void test_emplace()
|
|||
static_assert(has_index_emplace<variant<int>, 0>(0));
|
||||
static_assert(!has_type_emplace<variant<AllDeleted>, AllDeleted>(0));
|
||||
static_assert(!has_index_emplace<variant<AllDeleted>, 0>(0));
|
||||
static_assert(has_type_emplace<variant<int, AllDeleted>, int>(0));
|
||||
static_assert(has_index_emplace<variant<int, AllDeleted>, 0>(0));
|
||||
static_assert(has_type_emplace<variant<int, vector<int>, AllDeleted>, vector<int>>(0));
|
||||
static_assert(has_index_emplace<variant<int, vector<int>, AllDeleted>, 1>(0));
|
||||
|
||||
// The above tests only check the emplace members are available for
|
||||
// overload resolution. The following odr-uses will instantiate them:
|
||||
variant<int, vector<int>, AllDeleted> v;
|
||||
v.emplace<0>(1);
|
||||
v.emplace<int>(1);
|
||||
v.emplace<1>(1, 1);
|
||||
v.emplace<vector<int>>(1, 1);
|
||||
v.emplace<1>({1, 2, 3, 4});
|
||||
v.emplace<vector<int>>({1, 2, 3, 4});
|
||||
}
|
||||
|
||||
void test_triviality()
|
||||
|
|
|
@ -23,6 +23,7 @@
|
|||
#include <vector>
|
||||
#include <unordered_set>
|
||||
#include <memory_resource>
|
||||
#include <ext/throw_allocator.h>
|
||||
#include <testsuite_hooks.h>
|
||||
|
||||
using namespace std;
|
||||
|
@ -254,6 +255,41 @@ void emplace()
|
|||
VERIFY(&v.emplace<0>({1,2,3}) == &std::get<0>(v));
|
||||
VERIFY(&v.emplace<vector<int>>({1,2,3}) == &std::get<vector<int>>(v));
|
||||
}
|
||||
|
||||
{
|
||||
// Ensure no copies of the vector are made, only moves.
|
||||
// See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87431#c21
|
||||
|
||||
// static_assert(__detail::__variant::_Never_valueless_alt<vector<AlwaysThrow>>::value);
|
||||
variant<int, DeletedMoves, vector<AlwaysThrow>> v;
|
||||
v.emplace<2>(1);
|
||||
v.emplace<vector<AlwaysThrow>>(1);
|
||||
v.emplace<0>(0);
|
||||
|
||||
// To test the emplace(initializer_list<U>, Args&&...) members we
|
||||
// can't use AlwaysThrow because elements in an initialier_list
|
||||
// are always copied. Use throw_allocator instead.
|
||||
using Vector = vector<int, __gnu_cxx::throw_allocator_limit<int>>;
|
||||
// static_assert(__detail::__variant::_Never_valueless_alt<Vector>::value);
|
||||
variant<int, DeletedMoves, Vector> vv;
|
||||
Vector::allocator_type::set_limit(1);
|
||||
vv.emplace<2>(1, 1);
|
||||
Vector::allocator_type::set_limit(1);
|
||||
vv.emplace<Vector>(1, 1);
|
||||
Vector::allocator_type::set_limit(1);
|
||||
vv.emplace<0>(0);
|
||||
Vector::allocator_type::set_limit(1);
|
||||
vv.emplace<2>({1, 2, 3});
|
||||
Vector::allocator_type::set_limit(1);
|
||||
vv.emplace<Vector>({1, 2, 3, 4});
|
||||
try {
|
||||
Vector::allocator_type::set_limit(0);
|
||||
vv.emplace<2>(1, 1);
|
||||
VERIFY(false);
|
||||
} catch (__gnu_cxx::forced_error) {
|
||||
}
|
||||
VERIFY(vv.valueless_by_exception());
|
||||
}
|
||||
}
|
||||
|
||||
void test_get()
|
||||
|
|
Loading…
Reference in New Issue