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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Jun 9 08:22:19 PDT 2022

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
+_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

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list