From 45a3109dbb476d2b4a99830e2be38fd257127c89 Mon Sep 17 00:00:00 2001 From: Eric Fiselier Date: Fri, 13 Sep 2019 16:09:33 +0000 Subject: [PATCH] Recommit r370502: Make `vector` unconditionally move elements when exceptions are disabled. The patch was reverted due to some confusion about non-movable types. ie types that explicitly delete their move constructors. However, such types do not meet the requirement for `MoveConstructible`, which is required by `std::vector`: Summary: `std::vector` is free choose between using copy or move operations when it needs to resize. The standard only candidates that the correct exception safety guarantees are provided. When exceptions are disabled these guarantees are trivially satisfied. Meaning vector is free to optimize it's implementation by moving instead of copying. This patch makes `std::vector` unconditionally move elements when exceptions are disabled. This optimization is conforming according to the current standard wording. There are concerns that moving in `-fno-noexceptions`mode will be a surprise to users. For example, a user may be surprised to find their code is slower with exceptions enabled than it is disabled. I'm sympathetic to this surprised, but I don't think it should block this optimization. Reviewers: mclow.lists, ldionne, rsmith Reviewed By: ldionne Subscribers: zoecarver, christof, dexonsmith, libcxx-commits Tags: #libc Differential Revision: https://reviews.llvm.org/D62228 git-svn-id: https://llvm.org/svn/llvm-project/libcxx/trunk@371867 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/memory | 55 +++++++++++++++--- include/vector | 10 +++- ...xception_safety_exceptions_disabled.sh.cpp | 57 +++++++++++++++++++ .../resize.copy_only.pass.sh.cpp | 45 --------------- .../resize_not_move_insertable.fail.cpp | 45 +++++++++++++++ 5 files changed, 157 insertions(+), 55 deletions(-) create mode 100644 test/libcxx/containers/sequences/vector/exception_safety_exceptions_disabled.sh.cpp delete mode 100644 test/std/containers/sequences/vector/vector.modifiers/resize.copy_only.pass.sh.cpp create mode 100644 test/std/containers/sequences/vector/vector.modifiers/resize_not_move_insertable.fail.cpp diff --git a/include/memory b/include/memory index 321d5fee12..12573ca801 100644 --- a/include/memory +++ b/include/memory @@ -1507,6 +1507,31 @@ struct __is_default_allocator : false_type {}; template struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type {}; + + +template ::value && !__is_default_allocator<_Alloc>::value + > +struct __is_cpp17_move_insertable; +template +struct __is_cpp17_move_insertable<_Alloc, true> : std::true_type {}; +template +struct __is_cpp17_move_insertable<_Alloc, false> : std::is_move_constructible {}; + +template ::value && !__is_default_allocator<_Alloc>::value + > +struct __is_cpp17_copy_insertable; +template +struct __is_cpp17_copy_insertable<_Alloc, true> : __is_cpp17_move_insertable<_Alloc> {}; +template +struct __is_cpp17_copy_insertable<_Alloc, false> : integral_constant::value && + __is_cpp17_move_insertable<_Alloc>::value> + {}; + + + template struct _LIBCPP_TEMPLATE_VIS allocator_traits { @@ -1609,10 +1634,18 @@ struct _LIBCPP_TEMPLATE_VIS allocator_traits _LIBCPP_INLINE_VISIBILITY static void - __construct_forward(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2) + __construct_forward_with_exception_guarantees(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2) { + static_assert(__is_cpp17_move_insertable::value, + "The specified type does not meet the requirements of Cpp17MoveInsertible"); for (; __begin1 != __end1; ++__begin1, (void) ++__begin2) - construct(__a, _VSTD::__to_raw_pointer(__begin2), _VSTD::move_if_noexcept(*__begin1)); + construct(__a, _VSTD::__to_raw_pointer(__begin2), +#ifdef _LIBCPP_NO_EXCEPTIONS + _VSTD::move(*__begin1) +#else + _VSTD::move_if_noexcept(*__begin1) +#endif + ); } template @@ -1625,7 +1658,7 @@ struct _LIBCPP_TEMPLATE_VIS allocator_traits is_trivially_move_constructible<_Tp>::value, void >::type - __construct_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2) + __construct_forward_with_exception_guarantees(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2) { ptrdiff_t _Np = __end1 - __begin1; if (_Np > 0) @@ -1672,12 +1705,20 @@ struct _LIBCPP_TEMPLATE_VIS allocator_traits _LIBCPP_INLINE_VISIBILITY static void - __construct_backward(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __end2) + __construct_backward_with_exception_guarantees(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __end2) { + static_assert(__is_cpp17_move_insertable::value, + "The specified type does not meet the requirements of Cpp17MoveInsertable"); while (__end1 != __begin1) { - construct(__a, _VSTD::__to_raw_pointer(__end2-1), _VSTD::move_if_noexcept(*--__end1)); - --__end2; + construct(__a, _VSTD::__to_raw_pointer(__end2 - 1), +#ifdef _LIBCPP_NO_EXCEPTIONS + _VSTD::move(*--__end1) +#else + _VSTD::move_if_noexcept(*--__end1) +#endif + ); + --__end2; } } @@ -1691,7 +1732,7 @@ struct _LIBCPP_TEMPLATE_VIS allocator_traits is_trivially_move_constructible<_Tp>::value, void >::type - __construct_backward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __end2) + __construct_backward_with_exception_guarantees(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __end2) { ptrdiff_t _Np = __end1 - __begin1; __end2 -= _Np; diff --git a/include/vector b/include/vector index 2d83484aa2..be4894a89e 100644 --- a/include/vector +++ b/include/vector @@ -947,8 +947,10 @@ template void vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer& __v) { + __annotate_delete(); - __alloc_traits::__construct_backward(this->__alloc(), this->__begin_, this->__end_, __v.__begin_); + __alloc_traits::__construct_backward_with_exception_guarantees( + this->__alloc(), this->__begin_, this->__end_, __v.__begin_); _VSTD::swap(this->__begin_, __v.__begin_); _VSTD::swap(this->__end_, __v.__end_); _VSTD::swap(this->__end_cap(), __v.__end_cap()); @@ -963,8 +965,10 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer__alloc(), this->__begin_, __p, __v.__begin_); - __alloc_traits::__construct_forward(this->__alloc(), __p, this->__end_, __v.__end_); + __alloc_traits::__construct_backward_with_exception_guarantees( + this->__alloc(), this->__begin_, __p, __v.__begin_); + __alloc_traits::__construct_forward_with_exception_guarantees( + this->__alloc(), __p, this->__end_, __v.__end_); _VSTD::swap(this->__begin_, __v.__begin_); _VSTD::swap(this->__end_, __v.__end_); _VSTD::swap(this->__end_cap(), __v.__end_cap()); diff --git a/test/libcxx/containers/sequences/vector/exception_safety_exceptions_disabled.sh.cpp b/test/libcxx/containers/sequences/vector/exception_safety_exceptions_disabled.sh.cpp new file mode 100644 index 0000000000..1e9cbde011 --- /dev/null +++ b/test/libcxx/containers/sequences/vector/exception_safety_exceptions_disabled.sh.cpp @@ -0,0 +1,57 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// RUN: %build -fno-exceptions +// RUN: %run + +// UNSUPPORTED: c++98, c++03 + +// + +// Test that vector always moves elements when exceptions are disabled. +// vector is allowed to move or copy elements while resizing, so long as +// it still provides the strong exception safety guarantee. + +#include +#include + +#include "test_macros.h" + +#ifndef TEST_HAS_NO_EXCEPTIONS +#error exceptions should be disabled. +#endif + +bool allow_moves = false; + +class A { +public: + A() {} + A(A&&) { assert(allow_moves); } + explicit A(int) {} + A(A const&) { assert(false); } +}; + +int main(int, char**) { + std::vector v; + + // Create a vector containing some number of elements that will + // have to be moved when it is resized. + v.reserve(10); + size_t old_cap = v.capacity(); + for (int i = 0; i < v.capacity(); ++i) { + v.emplace_back(42); + } + assert(v.capacity() == old_cap); + assert(v.size() == v.capacity()); + + // The next emplace back should resize. + allow_moves = true; + v.emplace_back(42); + + return 0; +} diff --git a/test/std/containers/sequences/vector/vector.modifiers/resize.copy_only.pass.sh.cpp b/test/std/containers/sequences/vector/vector.modifiers/resize.copy_only.pass.sh.cpp deleted file mode 100644 index b24736e22b..0000000000 --- a/test/std/containers/sequences/vector/vector.modifiers/resize.copy_only.pass.sh.cpp +++ /dev/null @@ -1,45 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -// UNSUPPORTED: c++98, c++03 - -// RUN: %build -fno-exceptions -// RUN: %run - -// RUN: %build -// RUN: %run - -// UNSUPPORTED: c++98, c++03 - -// - -// Test that vector won't try to call the move constructor when resizing if -// the class has a deleted move constructor (but a working copy constructor). - -#include - -class CopyOnly { -public: - CopyOnly() { } - - CopyOnly(CopyOnly&&) = delete; - CopyOnly& operator=(CopyOnly&&) = delete; - - CopyOnly(const CopyOnly&) = default; - CopyOnly& operator=(const CopyOnly&) = default; -}; - -int main() { - std::vector x; - x.emplace_back(); - - CopyOnly c; - x.push_back(c); - - return 0; -} diff --git a/test/std/containers/sequences/vector/vector.modifiers/resize_not_move_insertable.fail.cpp b/test/std/containers/sequences/vector/vector.modifiers/resize_not_move_insertable.fail.cpp new file mode 100644 index 0000000000..2b4a5a9e2c --- /dev/null +++ b/test/std/containers/sequences/vector/vector.modifiers/resize_not_move_insertable.fail.cpp @@ -0,0 +1,45 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// UNSUPPORTED: c++98, c++03 + + +// + +// Test that vector produces a decent diagnostic for user types that explicitly +// delete their move constructor. Such types don't meet the Cpp17CopyInsertible +// requirements. + +#include + +template +class BadUserNoCookie { +public: + BadUserNoCookie() { } + + BadUserNoCookie(BadUserNoCookie&&) = delete; + BadUserNoCookie& operator=(BadUserNoCookie&&) = delete; + + BadUserNoCookie(const BadUserNoCookie&) = default; + BadUserNoCookie& operator=(const BadUserNoCookie&) = default; +}; + +int main() { + // expected-error@memory:* 2 {{"The specified type does not meet the requirements of Cpp17MoveInsertable"}} + { + + std::vector > x; + x.emplace_back(); + } + { + std::vector> x; + BadUserNoCookie<2> c; + x.push_back(c); + } + return 0; +}