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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Not a real review, just a couple drive-by comments. I'm unlikely to do a real review. :)



================
Comment at: libcxx/include/concepts:309
+    {
+      __y = _VSTD::exchange(__x, std::move(__y));
+    }
----------------
miscco wrote:
> 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`?
IMO yes; and also `std::move` should be `_VSTD::move`.


================
Comment at: libcxx/include/concepts:314
+
+namespace ranges {
+  inline constexpr auto swap = __swap::__fn{};
----------------
miscco wrote:
> 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
One thing that might be affected by this comment, and should certainly be tested, is whether `swap` itself is swappable:
```
decltype(std::ranges::swap) s1, s2;
std::ranges::swap(s1, s2);
```
I don't know whether it should be or not, but ISTR that this was the trouble spot for range-v3 `swap` a couple years ago.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162