[libcxx-commits] [PATCH] D127049: [libc++] Unwrap reverse_iterator<reverse_iterator<Iter>> in __unwrap_iter

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:14
 #include <__memory/pointer_traits.h>
 #include <iterator>
 #include <type_traits>
----------------
We should include `__iterator/iterator_traits.h` here instead, and then specialize `__unwrap_iter_impl` for `reverse_iterator<reverse_iterator<T>>` from `reverse_iterator.h`. That way, we don't need to pull the whole world into `unwrap_iter.h`, and we don't need to overload `__unwrap_iter(...)`, which wasn't meant for that (we used to overload it but then we moved to specializing `__unwrap_iter_impl` as a customization point instead).


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:104-108
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
+_OrigIter __rewrap_iter(_ReverseWrapper<_OrigIter> __iter1, _UnwrappedIter __iter2) {
+  return std::__rewrap_iter<_OrigIter, _ReverseWrapper<_UnwrappedIter>, _RewrapCount - 1>(
+    __iter1, _ReverseWrapper<_UnwrappedIter>(reverse_iterator<_UnwrappedIter>(__iter2)));
 }
----------------
I think it would be cleaner not to reuse the same `__rewrap_iter` overload set for this. We could have one "top level" entry point for `reverse_iterator<reverse_iterator<...>>` and then use a helper function to do the actual rewrapping.

One motivation for this is that I'd like to move to a specialization based customization point for this as well in the future.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy.pass.cpp:220
+  test_reverse_reverse_output<NotIncrementableIt<S>>();
 
   return true;
----------------
Since we handle more than 2 layers of `reverse_iterator`, we should test it.


================
Comment at: libcxx/test/libcxx/algorithms/unwrap_iter.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
`libcxx/test/libcxx/iterators/<something>.pass.cpp`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127049