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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__algorithm/move.h:92
 __move(_InIter __first, _Sent __last, _OutIter __result) {
-  auto __ret = std::__move_impl(std::__unwrap_iter(__first), std::__unwrap_iter(__last), std::__unwrap_iter(__result));
-  return std::make_pair(std::__rewrap_iter(__first, __ret.first), std::__rewrap_iter(__result, __ret.second));
-}
-
-template <class _InIter, class _Sent, class _OutIter>
-inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
-__enable_if_t<!is_copy_constructible<_InIter>::value
-           || !is_copy_constructible<_Sent>::value
-           || !is_copy_constructible<_OutIter>::value, pair<_InIter, _OutIter> >
-__move(_InIter __first, _Sent __last, _OutIter __result) {
-  return std::__move_impl(std::move(__first), std::move(__last), std::move(__result));
+  auto __first_un = std::__unwrap_iter(std::move(__first));
+  auto __result_un = std::__unwrap_iter(std::move(__result));
----------------
Please don't use `auto`, it doesn't help to make the code easier to understand.


================
Comment at: libcxx/include/__algorithm/move_backward.h:15
 #include <__utility/move.h>
+#include <__utility/pair.h>
 #include <cstring>
----------------
Why do I need `pair.h` here. Isn't this exported by `unwrap_iter`?


================
Comment at: libcxx/include/__filesystem/u8path.h:97
+  return u8path(
+      _VSTD::__unwrap_iter(_Traits::__range_begin(__s)).first, _VSTD::__unwrap_iter(_Traits::__range_end(__s)).first);
+#  else
----------------
As mentioned in D129040 I really like to avoid using `pair` when it makes the code unclear. Here it's quite hard to understand what `__unwrap_iter(...).first` is. Having a good name instead of `first` will help a lot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129039