[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/include/algorithm:1734
 {
-    return _VSTD::__copy(__unwrap_iter(__first), __unwrap_iter(__last), __unwrap_iter(__result));
+  if (__libcpp_is_constant_evaluated()) {
+    return _VSTD::__copy_constexpr(
----------------
ldionne wrote:
> 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?
@ldionne I took a stab at a approach that I think resembles `count_new.h`: https://reviews.llvm.org/differential/diff/225552/

The basic idea is to have an `ExecutionTracker` which can collect information about
the runtime execution path taken if `_LIBCPP_TESTING_EXECUTION_PATH` is defined.

1. This means that we end up with some garbage in the headers like:
```
#ifdef _LIBCPP_TESTING_EXECUTION_PATH
    globalExecutionPathTracker.fastPathCalled();
#endif

    const size_t __n = static_cast<size_t>(__last - __first);
    if (__n > 0)
        _VSTD::memmove(__result, __first, __n * sizeof(_Up));
    return __result + __n;
```
but perhaps that's not so different from other conditionals.

2. From the testing side, we have to do:
```
#define _LIBCPP_TESTING_EXECUTION_PATH
#include "track_execution_path.h"

#include <algorithm>
```
but perhaps this is not so different from `random_shuffle.cxx1z.pass.cpp` which does:
```
#define _LIBCPP_ENABLE_CXX17_REMOVED_RANDOM_SHUFFLE
#define _LIBCPP_DISABLE_DEPRECATION_WARNINGS

#include <algorithm>
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68837