[libcxx-commits] [PATCH] D68837: [libc++][P0202] Marked algorithms copy/copy_n/copy_if/copy_backward constexpr.

[Michael Park via Phabricator via libcxx-commits]

mpark marked an inline comment as done.
mpark added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp:22
+#if TEST_STD_VER > 17
+TEST_CONSTEXPR bool test_constexpr()
+{
----------------
ldionne wrote:
> zoecarver wrote:
> > It seems like these were already here. I think it would be really great if we could instead have a single test. For all these algorithms, what I would ideally like is just to change the return type to `bool`, add `constexpr`, and `return true` at the end. Then we can `static_assert` that `test` succeeds. If `assert` fails in the test, the `static_assert` will also fail. 
> > 
> > Maybe we can even develop a macro to wrap these functions and run them in both constexpr mode and normally. 
> We definitely need to run the tests in both runtime and compile-time worlds, since those exercise completely different things. Not only the code paths can be different (via `is_constant_evaluated`), but we also need to make sure the runtime code generated by the compiler agrees with the compile-time evaluation. So, running the tests in both `assert` and `static_assert` is needed IMO.
> 
> I don't think a lot of changes are required, because `assert(condition)` should be valid at compile-time if `condition` is true. I suggest something like the following:
> 
> ```
> #define ASSERT_CONSTEXPR(...) static_assert(((__VA_ARGS__),void(),true), "")
> ```
> 
> Then, this can be called as
> 
> ```
> ASSERT_CONSTEXPR(test<input_iterator<const int*>, output_iterator<int*> >());
> ```
> 
> without having to change `test<...>()` at all, I think. We could even go as far as to add `ASSERT_CONSTEXPR_AFTER_CXX17(...)`, etc.
> 
For now I've moved all of the cases from `main` into a `test()` function and called `test();` and `static_assert(test());`

Alternatively, we could maybe also add macro which does something ilke:
```
#if TEST_STD_VER > 17
  #define TEST_RUN_BOTH_AFTER_CXX17(...) (__VA_ARGS__); static_assert((__VA_ARGS__),void(),true)
#else
  #define TEST_RUN_BOTH_AFTER_CXX17(...) (__VA_ARGS__);
#endif
```
with which we could do something like:
```
    TEST_RUN_BOTH_AFTER_CXX17(test<input_iterator<const int*>, output_iterator<int*> >());
    TEST_RUN_BOTH_AFTER_CXX17(test<input_iterator<const int*>, input_iterator<int*> >());
    ...
```
The simplicity of the current approach seems fitting with the simplicity of the test suite of libc++ in general, but I don't know. I also don't know what a good name for `TEST_RUN_BOTH_AFTER_CXX17` would be 😕


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68837