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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/concepts:314
+
+namespace ranges {
+  inline constexpr auto swap = __swap::__fn{};
----------------
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.


================
Comment at: libcxx/include/concepts:314
+
+namespace ranges {
+  inline constexpr auto swap = __swap::__fn{};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162