Make `vector` unconditionally move elements when exceptions are disabled.

[EricWF]

Summary

std::vector<T> 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-noexceptionsmode 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.

[mclow.lists]

I'm not particularly in favor of this patch.
There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

I not that fond of -fno-exceptions in general, and I think that special-casing vector here is the wrong approach.
If we really want to embrace this (and I don't), I think we should change move_if_noexcept (and the other noexcept type traits) to return true when exceptions are disabled.

Also, these functions __construct_backward and __construct_forwards should be free functions, not non-standard extensions to allocator_traits (but that's existing code)

include/utility

284

Why an enum?
How about:

#ifdef _LIBCPP_NO_EXCEPTIONS
_LIBCPP_BOOL_CONSTANT(true) __exceptions_disabled;
#else
_LIBCPP_BOOL_CONSTANT(false) __exceptions_disabled;
#endif

[EricWF]

In D62228#1511999, @mclow.lists wrote:

I'm not particularly in favor of this patch.
There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

I not that fond of -fno-exceptions in general, and I think that special-casing vector here is the wrong approach.
If we really want to embrace this (and I don't), I think we should change move_if_noexcept (and the other noexcept type traits) to return true when exceptions are disabled.

You're proposing a non-standard and non-conforming extension. This change is not an extension. It is a conforming implementation of vector that is better optimized.
This change is orthogonal to the suggested non-conforming extension, and discussion about the extension is better had elsewhere.

Also, these functions __construct_backward and __construct_forwards should be free functions, not non-standard extensions to allocator_traits (but that's existing code)

They have an interesting relationship with the allocator, but I agree they're mis-placed.

include/utility

284

It's just a quick way to stash a value without having to worry about introducing a name with linkage.

[mclow.lists]

In D62228#1512483, @EricWF wrote:

In D62228#1511999, @mclow.lists wrote:

I'm not particularly in favor of this patch.
There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

I not that fond of -fno-exceptions in general, and I think that special-casing vector here is the wrong approach.
If we really want to embrace this (and I don't), I think we should change move_if_noexcept (and the other noexcept type traits) to return true when exceptions are disabled.

You're proposing a non-standard and non-conforming extension. This change is not an extension. It is a conforming implementation of vector that is better optimized.
This change is orthogonal to the suggested non-conforming extension, and discussion about the extension is better had elsewhere.

Are you seriously suggesting that any behavior under -fno-exceptions can be considered conforming?

[dexonsmith]

In D62228#1512483, @EricWF wrote:

In D62228#1511999, @mclow.lists wrote:

There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

I don't think this should be committed until there is consensus on the direction, and when there's contention the usual forum for establishing consensus is the dev list. Can you restart the discussion on libcxx-dev, or start a new one there?

[ldionne]

The approach I was more comfortable with was defining a macro that would be somewhat like noexcept(expression), but that would always return true when exceptions are disabled. This would be a library emulation of the compiler change we talked about on the list.

test/libcxx/utilities/utility/forward/extended_move_if_noexcept.sh.cpp

27–28

This comment should be in the header where __extended_move_if_noexcept is defined, not in this test.

[zoecarver]

include/utility

298

Missing public:. In C++03 these won't be public member types.

[EricWF]

Sorry, I think the previous patch was misleading. My ambitions don't extend beyond this small performance fix. There are no other places in libc++ that conditionally move.

I've removed __extended_move_if_noexcept because we've agreed the general abstraction it implements isn't what we want and isn't good generally.

Is this localized performance fix more acceptable? I don't think it violates the spirit of the previous conversation.

[rupprecht]

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

[ldionne]

In D62228#1658620, @rupprecht wrote:

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

I don't think this was intentional, and I think your example should be valid. I added the test you provided to the test suite in https://reviews.llvm.org/rCXX371067, and I temporarily reverted this commit in https://reviews.llvm.org/rCXX371068. We can take a look once @EricWF is back from vacation.

[rupprecht]

In D62228#1659300, @ldionne wrote:

In D62228#1658620, @rupprecht wrote:

It looks like this breaks uses of std::vector with classes that are copy only (moves disabled): https://godbolt.org/z/ct2GIe
gcc/libstdc++ or clang/libc++ w/ exceptions enabled both support it.

Is this breakage intentional?

I don't think this was intentional, and I think your example should be valid. I added the test you provided to the test suite in https://reviews.llvm.org/rCXX371067, and I temporarily reverted this commit in https://reviews.llvm.org/rCXX371068. We can take a look once @EricWF is back from vacation.

Thanks for the revert!

include/memory

 
     template <class _Ptr>
         _LIBCPP_INLINE_VISIBILITY
         static
         void
         __construct_forward(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2)
         {
             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),
+                        _VSTD::__extended_move_if_noexcept(*__begin1));
         }
 
     template <class _Tp>
         _LIBCPP_INLINE_VISIBILITY
         static
         typename enable_if
         <
             (__is_default_allocator<allocator_type>::value

     template <class _Ptr>
         _LIBCPP_INLINE_VISIBILITY
         static
         void
         __construct_backward(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __end2)
         {
             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),
+                        _VSTD::__extended_move_if_noexcept(*--__end1));
+              --__end2;
             }
         }
 
     template <class _Tp>
         _LIBCPP_INLINE_VISIBILITY
         static
         typename enable_if
         <

include/utility

typename enable_if<
     __is_swappable<_Tp>::value
 >::type
 swap(_Tp (&__a)[_Np], _Tp (&__b)[_Np]) _NOEXCEPT_(__is_nothrow_swappable<_Tp>::value)
 {
     _VSTD::swap_ranges(__a, __a + _Np, __b);
 }
 
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+struct __noexcept_traits {
+private:
 #ifndef _LIBCPP_CXX03_LANG
-typename conditional
-<
-    !is_nothrow_move_constructible<_Tp>::value && is_copy_constructible<_Tp>::value,
-    const _Tp&,
-    _Tp&&
->::type
-#else  // _LIBCPP_CXX03_LANG
-const _Tp&
+  enum {
+    __exceptions_disabled =
+#ifdef _LIBCPP_NO_EXCEPTIONS
+        true,
+#else
+        false,
 #endif
-move_if_noexcept(_Tp& __x) _NOEXCEPT
-{
-    return _VSTD::move(__x);
+    __move_if_noexcept = is_nothrow_move_constructible<_Tp>::value ||
+                         !is_copy_constructible<_Tp>::value,
+  };
+
+  template <bool _Pred>
+  using _SelectReturnType =
+      typename conditional<_Pred, _Tp&&, const _Tp&>::type;
+
+public:
+  using __move_if_noexcept_t = _SelectReturnType<__move_if_noexcept>;
+  using __extended_move_if_noexcept_t =
+      _SelectReturnType<__move_if_noexcept || __exceptions_disabled>;
+#else
+  typedef const _Tp& __move_if_noexcept_t;
+  typedef const _Tp& __extended_move_if_noexcept_t;
+#endif
+};
+
+template <class _Tp>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+typename __noexcept_traits<_Tp>::__move_if_noexcept_t
+move_if_noexcept(_Tp& __x) _NOEXCEPT {
+  return _VSTD::move(__x);
+}
+
+template <class _Tp>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+typename __noexcept_traits<_Tp>::__extended_move_if_noexcept_t
+__extended_move_if_noexcept(_Tp& __x) _NOEXCEPT {
 }
 
 #if _LIBCPP_STD_VER > 14
 template <class _Tp> constexpr add_const_t<_Tp>& as_const(_Tp& __t) noexcept { return __t; }
 template <class _Tp>                        void as_const(const _Tp&&) = delete;
 #endif
 
 struct _LIBCPP_TEMPLATE_VIS piecewise_construct_t { };

test/libcxx/containers/sequences/vector/exception_safety_exceptions_disabled.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+
+// <vector>
+
+// 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 <vector>
+#include <cassert>
+
+#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<A> 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;
+}

test/libcxx/utilities/utility/forward/extended_move_if_noexcept.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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 -fexceptions -DHAVE_EXCEPTIONS
+// RUN: %run
+// RUN: %build -fno-exceptions -DNO_EXCEPTIONS
+// RUN: %run
+
+// It's not clear if this test will always be able to override builds
+// where exceptions are disabled.
+//
+// UNSUPPORTED: libcpp-no-exceptions
+
+// <utility>
+
+// template <class T>
+// auto __extended_move_if_noexcept(_Tp&) -> see below
+
+// __extended_move_if_noexcept is similar to `move_if_noexcept` except
+// that it unconditionally moves when exceptions are disabled.
+
+// __extended_move_if_noexcept is used by containers that need to provide
+// the strong exception safety guarantee for certain modifying operations.
+//
+// When exceptions are disabled strong exception safety guarantee is trivially
+// satisfied and the container is allowed to optimize its implementation by
+// unconditionally moving.
+
+#include <utility>
+
+#include "test_macros.h"
+
+#if !defined(HAVE_EXCEPTIONS) && !defined(NO_EXCEPTIONS)
+#error Incorrect test setup
+#endif
+
+
+class A
+{
+    A(const A&);
+    A& operator=(const A&);
+public:
+
+    A() {}
+#if TEST_STD_VER >= 11
+    A(A&&) {}
+#endif
+};
+
+struct legacy
+{
+    legacy() {}
+    legacy(const legacy&);
+};
+
+template <class Expect, class Input>
+void test_standard_behavior(Input& in) {
+  ASSERT_SAME_TYPE(decltype(std::__extended_move_if_noexcept(in)), decltype(std::move_if_noexcept(in)));
+  ASSERT_SAME_TYPE(decltype(std::__extended_move_if_noexcept(in)), Expect);
+  ASSERT_NOEXCEPT(std::__extended_move_if_noexcept(in));
+}
+
+template <class Expect, class Input>
+void test_extended_behavior(Input& in) {
+  ASSERT_SAME_TYPE(decltype(std::__extended_move_if_noexcept(in)), Expect);
+  static_assert(!std::is_same<decltype(std::__extended_move_if_noexcept(in)), decltype(std::move_if_noexcept(in))>::value, "");
+  ASSERT_NOEXCEPT(std::__extended_move_if_noexcept(in));
+}
+
+int main(int, char**)
+{
+    int i = 0;
+    const int ci = 0;
+
+    legacy l;
+    A a;
+    const A ca;
+
+#if TEST_STD_VER >= 11
+    test_standard_behavior<int&&>(i);
+    test_standard_behavior<const int&&>(ci);
+    test_standard_behavior<A&&>(a);
+    test_standard_behavior<const A&&>(ca);
+#ifdef NO_EXCEPTIONS
+    test_extended_behavior<legacy&&>(l);
+#else
+    test_standard_behavior<const legacy&>(l);
+#endif
+#else  // C++ < 11
+    // In C++03 libc++ #define's decltype to be __decltype on clang and
+    // __typeof__ for other compilers. __typeof__ does not deduce the reference
+    // qualifiers and will cause this test to fail.
+    test_standard_behavior<const int&>(i);
+    test_standard_behavior<const int&>(ci);
+    test_standard_behavior<const A&>(a);
+    test_standard_behavior<const A&>(ca);
+    test_standard_behavior<const legacy&>(l);
+#endif
+
+#if TEST_STD_VER > 11
+    constexpr int i1 = 23;
+    constexpr int i2 = std::__extended_move_if_noexcept(i1);
+    static_assert(i2 == 23, "" );
+#endif
+
+  return 0;
+}