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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 2 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/include/concepts:270
+    !__unqualified_swappable_with<_Tp(&)[_Size], _Up(&)[_Size]> &&
+    extent_v<_Tp> == extent_v<_Up> &&
+    requires(_Tp(& __t)[_Size], _Up(& __u)[_Size], const __fn& __swap) {
----------------
miscco wrote:
> I think this is already covered by the explicit `_Size` parameter
Nice catch :-)


================
Comment at: libcxx/include/concepts:298
+    {
+      // FIXME(cjdb): replace loop with ranges::swap_ranges.
+      for (size_t __i = 0; __i < _Size; ++__i) {
----------------
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.


================
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`?
I'd prefer to faithfully follow the standard and trust an optimising compiler will deal with this appropriately :-)


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


================
Comment at: libcxx/test/std/concepts/lang/swappable.pass.cpp:38-39
+template <class T, class U>
+requires std::same_as<std::remove_cvref_t<T>, std::remove_cvref_t<U> >&&
+    std::swappable<std::remove_cvref_t<T> >
+        constexpr bool check_swap_21(T&& x, U&& y) {
----------------
miscco wrote:
> I find this supper hard to read I think we should enforce some linebreaks with `//`
clang-format, why you do dis?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97162