[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/test/std/utilities/utility/exchange/exchange.pass.cpp:45
+  TestExchangeNoexcept(TestExchangeNoexcept&&) noexcept(Move);
+  void operator=(const TestExchangeNoexcept&) noexcept(Assign);
+};
----------------
Quuxplusone wrote:
> 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`.)
Agree with the mildly weird, but they weren't strictly needed. In any case, I took your suggestion along with the rename.


================
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{};
----------------
Quuxplusone wrote:
> 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)));
> }
> ```
I'm good with those tests. I like the explicit use of moving `x`. Just adopted your tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111481