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

Michael Schellenberger Costa via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun Feb 21 23:59:07 PST 2021

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

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list