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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco accepted this revision.
miscco added a comment.

Not a maintainer, but this looks good to me



================
Comment at: libcxx/include/concepts:314
+
+namespace ranges {
+  inline constexpr auto swap = __swap::__fn{};
----------------
cjdb wrote:
> cjdb wrote:
> > tcanens wrote:
> > > Quuxplusone wrote:
> > > > cjdb wrote:
> > > > > 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
> > > > > The type lives in namespace `std::ranges::__swap`. I don't see any wording in [[customization.point.object]][1] or [[concepts.swappable]][2] that mandates this, though I do recall that range-v3 (and possibly cmcstl2) needed to do this in C++14 mode since there weren't inline variables at the time.
> > > > > 
> > > > > [1]: http://eel.is/c++draft/customization.point.object
> > > > > [2]: http://eel.is/c++draft/concepts.swappable
> > > > 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.
> > > [concepts.syn] mandates it.
> > > 
> > > This is required because otherwise you can't declare hidden friend `swap`s for anything in `namespace ranges`; the same namespace member can't be both a variable and a function, even if the function is otherwise invisible to ordinary lookup.
> > Yep, makes sense, done.
> Test added.
Note, that this does not really make sense, as the standard requires, that there is only a single instance of each CPO, but It definitly does not hurt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162