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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/concepts:298
+    {
+      // FIXME(cjdb): replace loop with ranges::swap_ranges.
+      for (size_t __i = 0; __i < _Size; ++__i) {
----------------
ldionne wrote:
> 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?
Thinking further, obviously we can't use `ranges::swap_ranges` before it has been implemented. I think we should add a TODO and come fix it later once it's implemented so that we are in line with the spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162