[libcxx-commits] [PATCH] D92255: [libc++] [P0202] constexpr set_union, set_difference, set_symmetric_difference, merge

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

Quuxplusone marked 5 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge.pass.cpp:48
+    friend TEST_CONSTEXPR bool operator<(const NonTrivial& a, const NonTrivial& b) {
+        return a.value / 10 < b.value / 10;
+    }
----------------
zoecarver wrote:
> Why do we divide this by 10? Let's make these tests as simple as possible. 
> 
> Nit: no friend, no pass-by-reference. 
Because we want to be able to treat 41 and 42 as "the same" for purposes of set merging, but then distinguish them in `lexicographical_compare`, to verify that they got merged in the proper order. (E.g., the `set_union` tests verify that indeed `set_union` is not commutative.)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:9
 
-// Older compilers don't support std::is_constant_evaluated
-// UNSUPPORTED: clang-4, clang-5, clang-6, clang-7, clang-8
-// UNSUPPORTED: apple-clang-9, apple-clang-10
+// UNSUPPORTED: clang-8
 
----------------
zoecarver wrote:
> What happened to all the other unsupported compilers? 
`// UNSUPPORTED: clang-8` was good enough for ee95c7020cebe4668a501f24305a76a044df5266, so I expect it's also good enough here.

I strongly suspect that the test harness somehow knows that `UNSUPPORTED: clang-8` implies `UNSUPPORTED: clang-7` and so on.
I have no particular explanation for what's the deal with `apple-clang-9, apple-clang-10`.
Alternatively, it's possible that no buildbot runs `clang-7` anymore?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:44
+
+struct NonTrivial {
+    int value;
----------------
zoecarver wrote:
> Can we put these all into a header?
We could, but they're small enough that I think that could wait until we want to use them elsewhere.

Also, note that the classes in the comparator tests are deliberately //not// `<`-comparable, just to verify that we never accidentally try to use their `operator<`. So we really have 4 different classes here, not just 2.

Background on my reluctance here: In the process of writing this PR I discovered `TrackedValue`, which @ldionne wrote back in 2015, put in a header, and then never used again. (I wanted to make `NonTrivial` just a `pair<Trivial, TrackedValue>` or something like that; but it turned out that `TrackedValue` wasn't constexpr, so I couldn't use it anyway.)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:61
+#if TEST_STD_VER >= 11
+static_assert(std::is_trivially_copyable<Trivial>::value, "");
+static_assert(!std::is_trivially_copyable<NonTrivial>::value, "");
----------------
zoecarver wrote:
> I think these will work in C++03. 
Officially, neither `static_assert` nor `is_trivially_copyable` are present in C++11. `test/libcxx/containers/sequences/vector.bool/trivial_for_purposes_of_call.pass.cpp` is unsupported in C++03 and I can't see any reason it would be unsupported //except// for its reliance on the `is_trivially_` type-traits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92255