[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 2 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/includes_comp.pass.cpp:39
 void
 test()
 {
----------------
zoecarver wrote:
> Can we make this test constexpr too? 
It turns out that we can! So in the latest revision of this PR, I'm proposing that in the specific case of `std::includes`, we can eliminate `test_constexpr` and do everything in `test`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/set_difference.pass.cpp:26
 
+#if TEST_STD_VER > 17
+constexpr bool test_constexpr() {
----------------
zoecarver wrote:
> What I'd really like to do is rather than adding a `test_constexpr` function, simply modify the existing "test" function to be constexpr and `return true` at the end. If there's an assertion that fires while the constexpr is being evaluated, it will no longer be a valid constexpr and therefore will fail to compile. I think we agreed we should (as much as possible) try to have the same tests for constexpr and non-constexpr. 
Yes, we should try to have the same tests run for constexpr and non-constexpr. My nod to that in this PR is to have all the main routines call //both// `assert(test_constexpr())` and `static_assert(test_constexpr())`. In general I didn't mess with the existing non-constexpr tests, except to change the `_comp` versions to pass a non-default comparator.

The stumbling block for messing with the existing non-constexpr tests is that we have to be careful not to break them in C++03 mode, which means no C++11-isms.

The existing tests also leave a lot to be desired — e.g. they don't test //at all// with non-trivially-copyable types — so I was also semi-consciously operating in the mode of "Someone will have to fix these tests later, but it doesn't have to be me right now." ;) I suppose I could scope-creep this PR to "rewrite all the set-combination-algo tests at the same time," and/or volunteer to write a followup PR for that.

(But if we're going to rewrite the set-algo tests, we should do it //not before// landing the missing constexpr set algos that are included in this PR. I don't want a period where the `set_union` tests have to look gratuitously different from the `set_intersection` tests. So rewriting the tests should happen either in this PR right here, or in a followup, but not in a prerequisite.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92255