[libcxx-commits] [PATCH] D99817: [libcxx] applies LWG3175

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp:671
+};
+constexpr Proxy proxy(A& a) { return Proxy{a}; }
+} // namespace N
----------------
Quuxplusone wrote:
> FWIW, you //could// just capitalize the letter `p` on line 680 and then you wouldn't need this function.
I found it surprising that [concept.swappable] did this, so I preserved it.


================
Comment at: libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp:681
+  value_swap(a1, proxy(a2));
+  return a1.m == -5 && a2.m == 5;
+}
----------------
Quuxplusone wrote:
> 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. ;)
> 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.


================
Comment at: libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp:684
+
+static_assert(CheckRegression());
+} // namespace LWG3175
----------------
Quuxplusone wrote:
> 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)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99817