[libcxx-commits] [PATCH] D97162: [libcxx] adds std::ranges::swap and std::swappable

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

I find the tests really hard to read and follow.  I think giving the lambdas an explicit name would be really beneficial.



================
Comment at: libcxx/include/concepts:270
+    !__unqualified_swappable_with<_Tp(&)[_Size], _Up(&)[_Size]> &&
+    extent_v<_Tp> == extent_v<_Up> &&
+    requires(_Tp(& __t)[_Size], _Up(& __u)[_Size], const __fn& __swap) {
----------------
I think this is already covered by the explicit `_Size` parameter


================
Comment at: libcxx/include/concepts:298
+    {
+      // FIXME(cjdb): replace loop with ranges::swap_ranges.
+      for (size_t __i = 0; __i < _Size; ++__i) {
----------------
I *think* this should not be done. We would lose a tone of throughput due to the additional checking done inside `ranges::swap_ranges`.

Is there a measurable benefit in using `ranges::swap_ranges`


================
Comment at: libcxx/include/concepts:309
+    {
+      __y = _VSTD::exchange(__x, std::move(__y));
+    }
----------------
While this is what the standard says, it boils down to the conventional swap implementation, which is better known. So should we simply avoid the function call and directly open code `swap`?


================
Comment at: libcxx/include/concepts:314
+
+namespace ranges {
+  inline constexpr auto swap = __swap::__fn{};
----------------
This *think* this must be inside another `inline namespace _meow` I tend to happily forget the exact reason

In general I would say we should put the whole CPO into the `ranges` namespace and then use an inline namespace here. It is a bit strange that the CPO itself lives outside of ranges


================
Comment at: libcxx/test/std/concepts/lang/swappable.pass.cpp:38-39
+template <class T, class U>
+requires std::same_as<std::remove_cvref_t<T>, std::remove_cvref_t<U> >&&
+    std::swappable<std::remove_cvref_t<T> >
+        constexpr bool check_swap_21(T&& x, U&& y) {
----------------
I find this supper hard to read I think we should enforce some linebreaks with `//`


================
Comment at: libcxx/test/std/concepts/lang/swappable.pass.cpp:67
+
+static_assert([] {
+  auto x = lvalue_adl_swappable(0);
----------------
Please wrap this into a separate function with a descriptive name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162