[libcxx] fixes `common_reference` requirement for `swappable_with`

[cjdb]

Summary

LWG3175 identifies that the common_reference requirement for
swappable_with is over-constraining and doesn't need to concern itself
with cv- or reference qualifiers.

[• Quuxplusone]

LGTM % nits.

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp

671

FWIW, you could just capitalize the letter p on line 680 and then you wouldn't need this function.

681

As-IMO-always with constexpr tests, it'd be better style to write

assert(a1.m == -5 && a2.m == 5);
return true;

This would incidentally match the snippet in https://cplusplus.github.io/LWG/issue3175 a little closer, too, FWIW.

However, maybe a bigger surprise with this test is that it's all constexpr. LWG3175 tests the non-compile-time case, and doesn't mention the constexpr keyword at all. Either way is probably equivalent for testing purposes, but this gets back to my usual "cruft" complaint: there's a lot of keywords not pulling their weight in here. ;)

684

Please add a "simple test" too. I think the simple test is just

static_assert(std::swappable_with<N::Proxy, N::A>);
static_assert(std::swappable_with<N::A, N::Proxy>);

(and if you wanted to eliminate the useless namespace N, that'd be cool; but I imagine you don't, because it's part of the LWG3175 snippet)

[cjdb]

gets CI passing

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp

671

I found it surprising that [concept.swappable] did this, so I preserved it.

681

As-IMO-always with constexpr tests, it'd be better style to write

assert(a1.m == -5 && a2.m == 5);
return true;

Do you have a technical reason for preferring this?

LWG3175 tests the non-compile-time case, and doesn't mention the constexpr keyword at all.

Eh, I suppose I could make this a run-time test if you insist, but the regression test is aimed at the concept std::swappable_with, not std::ranges::swap.

but this gets back to my usual "cruft" complaint: there's a lot of keywords not pulling their weight in here. ;)

I don't see the connection, but I also don't see which keywords aren't doing their job.

684

The crux of LWG3175 is that Proxy and A don't share a common reference (and so swappable_with<Proxy, A> == false). The defect was originally filed to correct the example only, and IIRC Casey noticed that the common_reference requirement was also out of whack.

I'm not sure there is a companion simple test for this, but I'm happy to be proven wrong.

[Mordante]

Please improve commit title and add a proper commit message to the review.
I also like to see it pass the CI.

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp

681

As-IMO-always with constexpr tests, it'd be better style to write

assert(a1.m == -5 && a2.m == 5);
return true;

Do you have a technical reason for preferring this?

I personally like the style with separate asserts better, especially if it gets more complex. For example I dislike https://reviews.llvm.org/D97115#inline-910325, but assert can't be used in C++11.

In this case I would like it to better match the wording of LWG3175.

[cjdb]

adds assert and makes commit message more descriptive

[cjdb]

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp

681

I don't like assert in constexpr diagnostics, but matching LWG3175 code is probably best.

[curdeius]

LGTM.

[cjdb]

rebases to activate CI (hopefully for the last time)

libcxx/docs/Cxx2aStatusIssuesStatus.csv

 "`3149 <https://wg21.link/LWG3149>`__","``DefaultConstructible``\  should require default initialization","Belfast","|Complete|","13.0"
 "","","","",""
 "`1203 <https://wg21.link/LWG1203>`__","More useful rvalue stream insertion","Prague","|Complete|","12.0"
 "`2859 <https://wg21.link/LWG2859>`__","Definition of *reachable* in [ptr.launder] misses pointer arithmetic from pointer-interconvertible object","Prague","",""
 "`3018 <https://wg21.link/LWG3018>`__","``shared_ptr``\  of function type","Prague","",""
 "`3050 <https://wg21.link/LWG3050>`__","Conversion specification problem in ``chrono::duration``\  constructor","Prague","",""
 "`3141 <https://wg21.link/LWG3141>`__","``CopyConstructible``\  doesn't preserve source values","Prague","",""
 "`3150 <https://wg21.link/LWG3150>`__","``UniformRandomBitGenerator``\  should validate ``min``\  and ``max``\ ","Prague","",""
-"`3175 <https://wg21.link/LWG3175>`__","The ``CommonReference``\  requirement of concept ``SwappableWith``\  is not satisfied in the example","Prague","",""
+"`3175 <https://wg21.link/LWG3175>`__","The ``CommonReference``\  requirement of concept ``SwappableWith``\  is not satisfied in the example","Prague","|Complete|","13.0"
 "`3194 <https://wg21.link/LWG3194>`__","``ConvertibleTo``\  prose does not match code","Prague","",""
 "`3200 <https://wg21.link/LWG3200>`__","``midpoint``\  should not constrain ``T``\  is complete","Prague","|Nothing To Do|",""
 "`3201 <https://wg21.link/LWG3201>`__","``lerp``\  should be marked as ``noexcept``\ ","Prague","|Complete|",""
 "`3226 <https://wg21.link/LWG3226>`__","``zoned_time``\  constructor from ``string_view``\  should accept ``zoned_time<Duration2, TimeZonePtr2>``\ ","Prague","",""
 "`3233 <https://wg21.link/LWG3233>`__","Broken requirements for ``shared_ptr``\  converting constructors","Prague","",""
 "`3237 <https://wg21.link/LWG3237>`__","LWG 3038 and 3190 have inconsistent PRs","Prague","",""
 "`3238 <https://wg21.link/LWG3238>`__","Insufficiently-defined behavior of ``std::function``\  deduction guides","Prague","",""
 "`3242 <https://wg21.link/LWG3242>`__","``std::format``\ : missing rules for ``arg-id``\  in ``width``\  and ``precision``\ ","Prague","",""

libcxx/include/concepts

namespace ranges::inline __cpo {
   inline constexpr auto swap = __swap::__fn{};
 } // namespace ranges::__cpo
 
 template<class _Tp>
 concept swappable = requires(_Tp& __a, _Tp& __b) { ranges::swap(__a, __b); };
 
 template<class _Tp, class _Up>
 concept swappable_with =
-  common_reference_with<_Tp&, _Up&> &&
+  common_reference_with<_Tp, _Up> &&
   requires(_Tp&& __t, _Up&& __u) {
     ranges::swap(_VSTD::forward<_Tp>(__t), _VSTD::forward<_Tp>(__t));
     ranges::swap(_VSTD::forward<_Up>(__u), _VSTD::forward<_Up>(__u));
     ranges::swap(_VSTD::forward<_Tp>(__t), _VSTD::forward<_Up>(__u));
     ranges::swap(_VSTD::forward<_Up>(__u), _VSTD::forward<_Tp>(__t));
   };
 
 // [concept.booleantestable]

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp

 // UNSUPPORTED: libcpp-no-concepts
 
 // template<class T, class U>
 // concept swappable_with = // see below
 
 #include <concepts>
 
 #include <array>
+#include <cassert>
 #include <deque>
 #include <forward_list>
 #include <list>
 #include <map>
 #include <memory>
 #include <optional>
 #include <set>
 #include <unordered_map>
struct swappable_with_rvalue_ref_to_volatile_s3 {
 
   friend void swap(swappable_with_rvalue_ref_to_volatile_s3 volatile&&,
                    swappable_with_rvalue_ref_to_volatile_s3 volatile&&);
   friend void swap(s3 volatile&&,
                    swappable_with_rvalue_ref_to_volatile_s3 volatile&&);
   friend void swap(swappable_with_rvalue_ref_to_volatile_s3 volatile&&,
                    s3 volatile&&);
 
-  operator s3 volatile &() volatile;
+  operator s3 volatile &&() volatile;
 };
 static_assert(
     std::swappable_with<swappable_with_rvalue_ref_to_volatile_s3 volatile&&,
                         s3 volatile&&>);
 
 struct swappable_with_rvalue_ref_to_cv_s3 {
   swappable_with_rvalue_ref_to_cv_s3(
       swappable_with_rvalue_ref_to_cv_s3 const volatile&);
   swappable_with_rvalue_ref_to_cv_s3(
       swappable_with_rvalue_ref_to_cv_s3 const volatile&&);
   swappable_with_rvalue_ref_to_cv_s3&
   operator=(swappable_with_rvalue_ref_to_cv_s3 const volatile&);
   swappable_with_rvalue_ref_to_cv_s3&
   operator=(swappable_with_rvalue_ref_to_cv_s3 const volatile&&);
 
   friend void swap(swappable_with_rvalue_ref_to_cv_s3 const volatile&&,
                    swappable_with_rvalue_ref_to_cv_s3 const volatile&&);
   friend void swap(s3 const volatile&&,
                    swappable_with_rvalue_ref_to_cv_s3 const volatile&&);
   friend void swap(swappable_with_rvalue_ref_to_cv_s3 const volatile&&,
                    s3 const volatile&&);
 
-  operator s3 const volatile &() const volatile;
+  operator s3 const volatile &&() const volatile;
 };
 static_assert(
     std::swappable_with<swappable_with_rvalue_ref_to_cv_s3 const volatile&&,
                         s3 const volatile&&>);
 
 namespace union_swap {
 union adl_swappable {
   int x;

 static_assert(!check_swappable_with<ImplicitlyDeletedMoveAssign,
                                     ImplicitlyDeletedMoveAssign>());
 static_assert(!check_swappable_with<NonMovable, NonMovable>());
 static_assert(
     !check_swappable_with<DerivedFromNonMovable, DerivedFromNonMovable>());
 static_assert(!check_swappable_with<HasANonMovable, HasANonMovable>());
 } // namespace types_with_purpose
 
+namespace LWG3175 {
+// Example taken directly from [concept.swappable]
+template <class T, std::swappable_with<T> U>
+constexpr void value_swap(T&& t, U&& u) {
+  std::ranges::swap(std::forward<T>(t), std::forward<U>(u));
+}
+
+template <std::swappable T>
+constexpr void lv_swap(T& t1, T& t2) {
+  std::ranges::swap(t1, t2);
+}
+
+namespace N {
+struct A {
+  int m;
+};
+struct Proxy {
+  A* a;
+  constexpr Proxy(A& a) : a{&a} {}
+  friend constexpr void swap(Proxy x, Proxy y) {
+    std::ranges::swap(*x.a, *y.a);
+  }
+};
+constexpr Proxy proxy(A& a) { return Proxy{a}; }
+} // namespace N
+
+[[nodiscard]] constexpr bool CheckRegression() {
+  int i = 1, j = 2;
+  lv_swap(i, j);
+  assert(i == 2 && j == 1);
+
+  N::A a1 = {5}, a2 = {-5};
+  value_swap(a1, proxy(a2));
+  return a1.m == -5 && a2.m == 5;
+}
+
+static_assert(CheckRegression());
+} // namespace LWG3175
+
 int main(int, char**) { return 0; }