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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:39
 "`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"
 "","","","","",""
----------------
`s/Complete/|Complete|/`


================
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<_T1>::value && is_nothrow_assignable<_T1&, _T2>::value)
 {
----------------
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`.


================
Comment at: libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp:45
+  TestExchangeNoexcept(TestExchangeNoexcept&&) noexcept(Move);
+  void operator=(const TestExchangeNoexcept&) noexcept(Assign);
+};
----------------
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`.)


================
Comment at: libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp:49-64
+  int v = 42;
+  static_assert(std::is_nothrow_move_constructible_v<int>);
+  static_assert(std::is_nothrow_assignable_v<int&, int>);
+  ASSERT_NOEXCEPT(std::exchange(v, {}));
+
+  using NothrowMoveConstructible = TestExchangeNoexcept<true, false>;
+  NothrowMoveConstructible nothrow_move_constructible{};
----------------
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)));
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111481