[libcxx-commits] [PATCH] D129039: [libc++] Use __unwrap_iter_impl for both unwrapping and rewrapping and allow passing move-only iterators in __unwrap_iter

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I understand the problem you're solving with move-only iterators and I do think it is worth solving. I'm just not sure how to do it best -- there's something I dislike about the approach taken here, IMO it creates a lot of complexity. I'll take another look and let you know if I can think of another approach.



================
Comment at: libcxx/include/__algorithm/copy.h:61
           __enable_if_t<is_same<typename remove_const<__iter_value_type<_InIter> >::type, __iter_value_type<_OutIter> >::value
                       && __is_cpp17_contiguous_iterator<_InIter>::value
                       && __is_cpp17_contiguous_iterator<_OutIter>::value
----------------
Can you please explain the context around this patch in the commit message? Like you just did verbally during live review.


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:25
 
 // The job of __unwrap_iter is to lower contiguous iterators (such as
 // vector<T>::iterator) into pointers, to reduce the number of template
----------------
Can you please update this documentation?


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:41
 template <class _Iter, bool = __is_cpp17_contiguous_iterator<_Iter>::value>
 struct __unwrap_iter_impl {
+  static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Iter __rewrap(_Iter, _Iter __iter) { return __iter; }
----------------
I like that you are grouping `rewrap` and `unwrap` into the same struct. However, we should (not necessarily in this patch) change the name from `__unwrap_iter_impl` to something else that is more accurate, such as `__wrapped_iterator_traits` (strawman proposal, this isn't perfect either).

Can you please add a TODO?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129039