[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*> >();
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Why is this needed?
> Suppose we remove the guard. And suppose we compile this file as C++14. Then, `do_tests` is marked `constexpr`, but it unconditionally calls `test<input_iterator<const int*>, input_iterator<const int*>>()`, which never returns a compile-time-constant result. My understanding of C++ formalities is that this would make it IFNDR.
> 
> Adding the `bool` parameter makes it probably not IFNDR, since now there's a compile-time-constant path through this function even in C++14.
> 
> Constexpr is a huge pain. :P
I see what you're getting at. `std::includes` is marked as `_LIBCPP_CONSTEXPR_AFTER_CXX17` so let's change `TEST_CONSTEXPR_CXX14` to `TEST_CONSTEXPR_CXX20` (or whatever the equivalent is).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/includes_comp.pass.cpp:39
 void
 test()
 {
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > 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. 
> Well, look at the existing "merge.pass.cpp" for example. It's extremely silly. The test for `std::includes` was really the one special case, where the existing test (was still very silly, but) happened to be constexpr-friendly already.
What about `merge.pass.cpp` isn't constexpr friendly? The only thing I see is `std::reverse` which we should make constexpr anyway, maybe as part of this patch or a different patch that lands before this one. Otherwise, maybe it makes sense to just write out std::reverse inline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92255