[libcxx-commits] [PATCH] D101404: [libcxx] Fix __wrap_iter to work with to_address.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

> Can we just remove `__is_cpp17_contiguous_iterator` altogether? It's only used in two places

Maybe, but I doubt it. The places it's used are specifically to implement the lowering of `std::copy` into `memmove`: https://godbolt.org/z/5P9TjGza4
For performance, we //really// want to be able to detect "this specific iterator is known contiguous" in C++17/14/11/03 mode.
Now, can we change `__is_cpp17_contiguous_iterator` to rely on `std::contiguous_iterator` in C++20 and later? I would say yes. But we can't just make it `false` in C++17-and-earlier; it's still got to do the right thing for known-contiguous iterators in all modes. Unless we decide we don't care about that optimization. There's definitely (and IMHO unfortunately) precedent for libc++ saying "optimization isn't our department, let's just ask Clang to optimize better," but at least I wouldn't want to unilaterally decide that we don't care about this specific optimization.

> Note: the only place [...] fails in the whole codebase is in [...] test/libcxx/iterators/contiguous_iterators.pass.cpp.

This line, right?:

  static_assert((!std::__is_cpp17_contiguous_iterator<std::__wrap_iter<std::reverse_iterator<T *> > >::value), "");

I'd be 100% cool with eliminating that line from the test, because I think it violates the unstated precondition on `__wrap_iter`'s `_Iter` argument. (And then we can document that precondition and enforce it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404