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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/includes_comp.pass.cpp:57
 {
+  if (do_it) {
     test<input_iterator<const int*>, input_iterator<const int*> >();
----------------
Why is this needed?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/includes_comp.pass.cpp:93
+{
+    assert(do_tests(true));
 #if TEST_STD_VER > 17
----------------
There's no reason to assert here. The only reason we `static_assert` in the other one is to force constant evaluation. 


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/includes_comp.pass.cpp:39
 void
 test()
 {
----------------
Quuxplusone wrote:
> 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`.
This is great! Let's do this for all the tests. 


================
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() {
----------------
Quuxplusone wrote:
> 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.)
> 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.

What C++11-isms are you getting stuck on? 

IMHO the tests for this patch should be pretty simple. I think it would be great to improve the tests as you suggested, but I agree, that doesn't need to be part of this patch. I think/hope you can just follow the model in `includes.pass.cpp`. There's no regression for non-constexpr and we're testing the same thing in both "modes."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92255