[libcxx-commits] [PATCH] D99817: [libcxx] fixes `common_reference` requirement for `swappable_with`

[Christopher Di Bella via Phabricator via libcxx-commits]

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


================
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;
+}
----------------
Mordante wrote:
> cjdb wrote:
> > 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.
> > > 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.
I don't like `assert` in constexpr diagnostics, but matching LWG3175 code is probably best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99817