[libcxx-commits] [PATCH] D111481: [libc++] P2401: conditional noexcept for std::exchange

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 4 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/include/__utility/exchange.h:26
 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_v<_T1> && is_nothrow_assignable_v<_T1&, _T2>)
 {
----------------
Mordante wrote:
> `is_nothrow_move_constructible_v` requires C++17.
Fixed, thanks!


================
Comment at: libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp:16
 //    constexpr T            // constexpr after C++17
 //    exchange(T& obj, U&& new_value);
 
----------------
Mordante wrote:
> Please update the synopsis. Here and in the `<utility>` header.
Updated the synopsis, including the note about `constexpr` after C++17 in the `<utility>` header.


================
Comment at: libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp:54
+
+  TestExchangeNoexcept<true, false> nothrow_move_constructible{};
+  static_assert(std::is_nothrow_move_constructible_v<decltype(nothrow_move_constructible)>);
----------------
Mordante wrote:
> Can you make a typedef for this type so the `decltype`s below aren't required.
Added type aliases to clean things up.


================
Comment at: libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp:110
 
+    test_noexcept();
+    static_assert(test_noexcept());
----------------
Mordante wrote:
> Maybe move this to line 105, so the `static_assert` test can be guarded by the `TEST_STD_VER` at line 106.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111481/new/

https://reviews.llvm.org/D111481