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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % nits.



================
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
----------------
FWIW, you //could// just capitalize the letter `p` on line 680 and then you wouldn't need this function.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99817