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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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



================
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;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99817