[libcxx-commits] [PATCH] D68837: [libc++][P0202] Marked algorithms copy/copy_n/copy_if/copy_backward constexpr.
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Oct 16 07:04:50 PDT 2019
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
More information about the libcxx-commits
mailing list