[libcxx-commits] [PATCH] D122982: [libc++] Implement ranges::copy{, _n, _if, _backward}

Konstantin Varlamov via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Apr 5 16:57:27 PDT 2022

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.

Comment at: libcxx/include/__algorithm/copy.h:44
+  if (__libcpp_is_constant_evaluated()
+      && !is_trivially_copyable<_ValueT>::value
var-const wrote:
> Can you please add a comment to explain the GCC workaround?
Sorry about nitpicking, but reading the comment, I think it would be better phrased as a TODO (something like `Remove this workaround once GCC supports using __builtin_memmove in constexpr contexts`).

Comment at: libcxx/include/__algorithm/ranges_copy_if.h:43
+  __go(_InIter __first, _Sent __last, _OutIter __result, _Pred& __pred, _Proj& __proj) {
+    for (; __first != __last; ++__first) {
+      if (std::invoke(__pred, std::invoke(__proj, *__first))) {
philnik wrote:
> var-const wrote:
> > Question: why doesn't this algorithm delegate to the non-ranges version (is it to avoid having to deal with the projection)?
> Why would it? It's a really simple algorithm and has no pitfalls AFAIK.
I don't disagree -- I'm just wondering because it's inconsistent with what `ranges::copy` does, which is to delegate.

Comment at: libcxx/include/algorithm:60
   template<input_iterator I, sentinel_for<I> S, class T, class Proj = identity>
     requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
Hmm, looks accidentally deleted?

Comment at: libcxx/include/algorithm:113
 template <class InputIterator, class Predicate>
     constexpr bool     // constexpr in C++20
Looks accidentally deleted?

Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:50
+template <class In, class Out, class Sent = In>
+constexpr void test_iterators() {
+  { // simple test
philnik wrote:
> var-const wrote:
> > I think the current tests always use trivially copyable values -- can we add at least one test case where values are not trivially copyable?
> `CopyOnce` and `OnlyBackwardsCopyable` aren't trivial.
Yes, but those tests are focused on checking something else. I'd prefer to have a dedicated test for this, even if it's somewhat redundant with the other tests.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list