[libc++] P2401: conditional noexcept for std::exchange

[jloser]

Summary

Implement P2401 which adds a noexcept specification to
std::exchange. Treated as a defect fix which is the motivation for
applying this change to all standards mode rather than just C++23 or
later as the paper suggests.

[Mordante]

Thanks for working on this! It seems you use C++17 only code in C++14 mode.

libcxx/include/__utility/exchange.h

26–27

is_nothrow_move_constructible_v requires C++17.

libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp

16–17

Please update the synopsis. Here and in the <utility> header.

54

Can you make a typedef for this type so the decltypes below aren't required.

114

Maybe move this to line 105, so the static_assert test can be guarded by the TEST_STD_VER at line 106.

[jloser]

Address review comments

[jloser]

libcxx/include/__utility/exchange.h

26–27

Fixed, thanks!

libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp

16–17

Updated the synopsis, including the note about constexpr after C++17 in the <utility> header.

54

Added type aliases to clean things up.

114

The above TEST_STD_VER is > 17 but since this noexcept goes back to C++14 (when std::exchange was introduced), I think it's best to leave it here.

[• Quuxplusone]

Requesting changes, but pretty minor ones. The idea LGTM in general.

libcxx/docs/Status/Cxx2bPapers.csv

39

s/Complete/|Complete|/

libcxx/include/__utility/exchange.h

26–27

I'd prefer to see the noexcept-clause broken onto the next line (and indented), for readability:

_T1 exchange(_T1& __obj, _T2&& __new_value)
    noexcept(is_nothrow_move_constructible<_T1>::value && is_nothrow_assignable<_T1&, _T2>::value)

There's also a pre-existing bogus extra space in _T2 && __new_value.

libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp

45

Mildly weird that you're using move-construction but copy-assignment. (It'd be less weird if you were testing all 4-or-whatever permutations of move and copy operations, but you're not.)
I suggest more like

template<bool Move, bool Assign>
struct TestNoexcept {
  TestNoexcept() = default;
  TestNoexcept(const TestNoexcept&);
  TestNoexcept(TestNoexcept&&) noexcept(Move);
  TestNoexcept& operator=(const TestNoexcept&);
  TestNoexcept& operator=(TestNoexcept&&) noexcept(Assign);
};

(No need for Exchange in the name of the class, because this whole file is about std::exchange.)

49–64

Contrary to @Mordante's request, I'd prefer to simplify this code until no typedef is needed. I'd write the whole function's body as simply

{
    int x = 42;
    ASSERT_NOEXCEPT(std::exchange(x, 42));
}
{
    TestNoexcept<true, true> x;
    ASSERT_NOEXCEPT(std::exchange(x, std::move(x)));
    ASSERT_NOT_NOEXCEPT(std::exchange(x, x));  // copy-assignment is not noexcept
}
{
    TestNoexcept<true, false> x;
    ASSERT_NOT_NOEXCEPT(std::exchange(x, std::move(x)));
}
{
    TestNoexcept<false, true> x;
    ASSERT_NOT_NOEXCEPT(std::exchange(x, std::move(x)));
}

[jloser]

Address Arthur's feedback

[jloser]

libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp

45

Agree with the mildly weird, but they weren't strictly needed. In any case, I took your suggestion along with the rename.

49–64

I'm good with those tests. I like the explicit use of moving x. Just adopted your tests.

[jloser]

Empty message for static_assert so test works in C++14 mode

[Mordante]

A few small remarks remaining.

libcxx/include/utility

187

Please add the constexpr in the signature.

Please add noexcept in C++23. This only is required in C++23. Since it's allowed to strengthen noexcept I don't mind it available in earlier versions.

libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp

49–64

Contrary to @Mordante's request, I'd prefer to simplify this code until no typedef is needed. I'd write the whole function's body as simply

{
    int x = 42;
    ASSERT_NOEXCEPT(std::exchange(x, 42));
}

I'm fine with this version without typedefs, my main issue was the amount of decltypes in the original version.

114

I see the test executes no code, otherwise it couldn't execute the test in constexpr until C++20. ASSERT_NOEXCEPT is just a static_assert. So there's no real need to test this function twice.

[jloser]

Add constexpr noexcept to synopsis
Remove runtime call of test_noexcept() in test

[jloser]

libcxx/include/utility

187

Added both constexpr and noexcept.

libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp

114

Valid point -- just removed the runtime function call.

[Mordante]

LGTM provided the CI passes. (Some of our builders are currently offline, I already notified the maintainer of these nodes.)

[jloser]

ping @Quuxplusone. The test is as you proposed, but wanted your approval before I land this.

[• Quuxplusone]

LGTM!

libcxx/docs/Status/Cxx2bPapers.csv

 "`P1272 <https://wg21.link/P1272>`__","LWG","Byteswapping for fun&&nuf","October 2021","",""
 "`P1675 <https://wg21.link/P1675>`__","LWG","``rethrow_exception`` must be allowed to copy","October 2021","",""
 "`P2077 <https://wg21.link/P2077>`__","LWG","Heterogeneous erasure overloads for associative containers","October 2021","",""
 "`P2251 <https://wg21.link/P2251>`__","LWG","Require ``span`` & ``basic_string_view`` to be Trivially Copyable","October 2021","|Complete|","14.0"
 "`P2301 <https://wg21.link/P2301>`__","LWG","Add a ``pmr`` alias for ``std::stacktrace``","October 2021","",""
 "`P2321 <https://wg21.link/P2321>`__","LWG","``zip``","October 2021","",""
 "`P2340 <https://wg21.link/P2340>`__","LWG","Clarifying the status of the 'C headers'","October 2021","",""
 "`P2393 <https://wg21.link/P2393>`__","LWG","Cleaning up ``integer``-class types","October 2021","",""
-"`P2401 <https://wg21.link/P2401>`__","LWG","Add a conditional ``noexcept`` specification to ``std::exchange``","October 2021","",""
+"`P2401 <https://wg21.link/P2401>`__","LWG","Add a conditional ``noexcept`` specification to ``std::exchange``","October 2021","|Complete|","14.0"
 "","","","","",""

libcxx/include/__utility/exchange.h

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___UTILITY_EXCHANGE_H
 #define _LIBCPP___UTILITY_EXCHANGE_H
 
 #include <__config>
 #include <__utility/forward.h>
 #include <__utility/move.h>
+#include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if _LIBCPP_STD_VER > 11
 template<class _T1, class _T2 = _T1>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-_T1 exchange(_T1& __obj, _T2 && __new_value)
+_T1 exchange(_T1& __obj, _T2&& __new_value)
+    noexcept(is_nothrow_move_constructible<_T1>::value && is_nothrow_assignable<_T1&, _T2>::value)
 {
     _T1 __old_value = _VSTD::move(__obj);
     __obj = _VSTD::forward<_T2>(__new_value);
     return __old_value;
 }
 #endif // _LIBCPP_STD_VER > 11
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___UTILITY_EXCHANGE_H

libcxx/include/utility

template<class T, T N>
   using make_integer_sequence = integer_sequence<T, 0, 1, ..., N-1>;
 template<size_t N>
   using make_index_sequence = make_integer_sequence<size_t, N>;
 
 template<class... T>
   using index_sequence_for = make_index_sequence<sizeof...(T)>;
 
 template<class T, class U=T>
-    T exchange(T& obj, U&& new_value);
+    constexpr T exchange(T& obj, U&& new_value)
+      noexcept(is_nothrow_move_constructible<T>::value && is_nothrow_assignable<T&, U>::value); // constexpr in C++17, noexcept in C++23
 
 // 20.2.7, in-place construction // C++17
 struct in_place_t {
   explicit in_place_t() = default;
 };
 inline constexpr in_place_t in_place{};
 template <class T>
   struct in_place_type_t {

libcxx/test/std/utilities/utility/exchange/exchange.pass.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11
 // <utility>
 
 // exchange
 
 // template<class T, class U=T>
 //    constexpr T            // constexpr after C++17
-//    exchange(T& obj, U&& new_value);
+//    exchange(T& obj, U&& new_value)
+//      noexcept(is_nothrow_move_constructible<T>::value && is_nothrow_assignable<T&, U>::value);
 
 #include <utility>
 #include <cassert>
 #include <string>
 
 #include "test_macros.h"
 
 #if TEST_STD_VER > 17
 TEST_CONSTEXPR bool test_constexpr() {
     int v = 12;
 
     if (12 != std::exchange(v,23) || v != 23)
         return false;
 
     if (23 != std::exchange(v,static_cast<short>(67)) || v != 67)
         return false;
 
     if (67 != std::exchange<int, short>(v, {}) || v != 0)
         return false;
     return true;
     }
 #endif
 
+template<bool Move, bool Assign>
+struct TestNoexcept {
+    TestNoexcept() = default;
+    TestNoexcept(const TestNoexcept&);
+    TestNoexcept(TestNoexcept&&) noexcept(Move);
+    TestNoexcept& operator=(const TestNoexcept&);
+    TestNoexcept& operator=(TestNoexcept&&) noexcept(Assign);
+};
 
+constexpr bool test_noexcept() {
+  {
+    int x = 42;
+    ASSERT_NOEXCEPT(std::exchange(x, 42));
+  }
+  {
+    TestNoexcept<true, true> x;
+    ASSERT_NOEXCEPT(std::exchange(x, std::move(x)));
+    ASSERT_NOT_NOEXCEPT(std::exchange(x, x)); // copy-assignment is not noexcept
+  }
+  {
+    TestNoexcept<true, false> x;
+    ASSERT_NOT_NOEXCEPT(std::exchange(x, std::move(x)));
+  }
+  {
+    TestNoexcept<false, true> x;
+    ASSERT_NOT_NOEXCEPT(std::exchange(x, std::move(x)));
+  }
+
+  return true;
+}
 
 int main(int, char**)
 {
     {
     int v = 12;
     assert ( std::exchange ( v, 23 ) == 12 );
     assert ( v == 23 );
     assert ( std::exchange ( v, static_cast<short>(67) ) == 23 );
int main(int, char**)
     assert ( std::exchange ( s3, "" ) == s2 );
     assert ( s3.size () == 0 );
     }
 
 #if TEST_STD_VER > 17
     static_assert(test_constexpr());
 #endif
 
+    static_assert(test_noexcept(), "");
+
   return 0;
 }