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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

> 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.

The thing I'm more annoyed about is that it is used in one place and spread across four files. Here's what I suggest: we "move" `__is_cpp17_contiguous_iterator` to `algorithm` and rename it to something like `__is_pointer_like_iterator` or `__is_any_contiguous_iterator` and define it as so (pseudo code):

  #if C++20
  template<class T>
  constexpr inline bool __is_pointer_like_iterator = std::contiguous_iterator<T>;
  #else
  template<class T>
  constexpr inline bool __is_pointer_like_iterator = requires { typename pointer_traits<T>::element_type };
  #endif

WDYT?

> This line, right?:

Yes, and looking more closely the only lines it fails on are similar assertions that check what types //aren't// contiguous iterators.

> 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.)

:) Great, I agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404