[libcxx-commits] [PATCH] D80558: Add constexpr to pair

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Mostly LGTM with nits.



================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp:27
+  int moved = 0;
+  TEST_CONSTEXPR_CXX20 void reset() { copied = moved = 0; }
+  TEST_CONSTEXPR_CXX20 CountAssign() = default;
----------------
This isn't used.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair_U_V.pass.cpp:40
     {
+#if TEST_STD_VER >= 20
+       using C = ConstexprTestTypes::TestType;
----------------
miscco wrote:
> I am really not too happy with this dance. I guess one should add one additional section for C++20 only
I think this is acceptable, we do it in a bunch of other places for testing constexpr. I do agree that something in the test runner would be better, but eh.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/swap.pass.cpp:24
+    TEST_CONSTEXPR_CXX20 S(int j) : i(j) {}
     S * operator& () { assert(false); return this; }
     S const * operator& () const { assert(false); return this; }
----------------
miscco wrote:
> Could we remove those by the way? I do not see any use for them
Do you mean remove `S` altogether? It's used below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80558