[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 a comment.
This revision now requires changes to proceed.

Looks good, except for the duplicated `test()` functions. I'd also like to have a short discussion about how to test this (and more importantly the lack of regression w.r.t. codegen). Apart from that, I think this is good to go.



================
Comment at: libcxx/include/algorithm:1734
 {
-    return _VSTD::__copy(__unwrap_iter(__first), __unwrap_iter(__last), __unwrap_iter(__result));
+  if (__libcpp_is_constant_evaluated()) {
+    return _VSTD::__copy_constexpr(
----------------
This LGTM. However, one thing I just thought about is that we don't really have a way of making sure that libc++ is calling the right code at runtime vs compile-time. What I mean is that with the introduction of `__libcpp_is_constant_evaluated()`, we're adding a new code path that is equivalent but slower. Given some programming error (such as the previous way of dispatching on `__libcpp_is_constant_evaluated()`, we could conceivably end up in a situation where we always fall back to the slower (but still correct) implementation, and our tests wouldn't currently catch that.

I can't think of a way to test this at the moment, however I thought I'd at least mention it because it's a new class of potential bugs we're introducing (and against which we don't have good defence).

__Edit:__

Since this is really a property of the code generation, perhaps one way we could test this is by having some kind of `.sh.cpp` test that checks the codegen of a function calling `std::copy`, and confirming that for the appropriate iterator types it generates a call to `memcpy`. This would go in `test/libcxx` (since it's a property of our implementation only). Thoughts?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp:23
+TEST_CONSTEXPR_CXX20 void
 test()
 {
----------------
You seem to have two functions called `test()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68837