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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/concepts:298
+    {
+      // FIXME(cjdb): replace loop with ranges::swap_ranges.
+      for (size_t __i = 0; __i < _Size; ++__i) {
----------------
cjdb wrote:
> miscco wrote:
> > 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`
> http://eel.is/c++draft/concept.swappable#2.2 explicitly mentions it as the call, but if we can skip the call, I'm all for it.
I don't understand Michael's comment about the loss of throughput for using `swap_ranges` here. It seems to me like that would be the correct way of implementing this. Why reinvent the wheel?


================
Comment at: libcxx/include/concepts:309
+  };
+}
+
----------------
`// end namespace ranges::__swap`


================
Comment at: libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp:593
+
+#ifndef _LIBCPP_HAS_NO_THREADS
+static_assert(!check_swappable_with<std::lock_guard<std::mutex>,
----------------
EricWF wrote:
> Can we write these tests in a way that makes them non-threading dependent.
> 
I don't think so since he's explicitly trying to exercise these types, which aren't provided when `_LIBCPP_HAS_NO_THREADS`. Better would be to have a test-suite version of the macro (to express the fact that the test suite supports being used when threads are not available), but that's for a different patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162