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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/algorithm:1709
 <
+    !__libcpp_is_constant_evaluated() &&
     is_same<typename remove_const<_Tp>::type, _Up>::value &&
----------------
If I'm not mistaken, `__libcpp_is_constant_evaluated()` is always true here, because the result is always required to be a constant expression (since it's a NTTP). This is one of the many pitfalls of this function.

Instead,I think you need to branch in the main `copy()` function, like so:

```
template <class _InputIterator, class _OutputIterator>
inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
_OutputIterator
copy(_InputIterator __first, _InputIterator __last, _OutputIterator __result)
{
    if (__libcpp_constant_evaluated()) {
        return _VSTD::__copy_naive(__unwrap_iter(__first), __unwrap_iter(__last), __unwrap_iter(__result));
    }
    else {
        return _VSTD::__copy(__unwrap_iter(__first), __unwrap_iter(__last), __unwrap_iter(__result));
    }
}
```

Or something along those lines, you get the point.



================
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()
+{
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68837