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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Mar 4 14:46:16 PST 2021

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
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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list