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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

A few small remarks remaining.



================
Comment at: libcxx/include/utility:187
+    T exchange(T& obj, U&& new_value)
+      noexcept(is_nothrow_move_constructible<T>::value && is_nothrow_assignable<T&, U>::value); // constexpr in C++17
 
----------------
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.


================
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{};
----------------
jloser wrote:
> 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.
> 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 `typedef`s, my main issue was the amount of `decltype`s in the original version.




================
Comment at: libcxx/test/std/utilities/utility/exchange/exchange.pass.cpp:110
 
+    test_noexcept();
+    static_assert(test_noexcept());
----------------
jloser wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111481