[libcxx-commits] [PATCH] D129040: [libc++] Fix unwrapping ranges with different iterators and sentinels

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I really would like to encourage you to add a bit more comments to new code. Historically we haven't done that a lot, but IMO that makes some older parts of the code harder to understand.



================
Comment at: libcxx/include/__algorithm/copy.h:13
 #include <__algorithm/unwrap_iter.h>
+#include <__algorithm/unwrap_range.h>
 #include <__config>
----------------
Please also add this to the top level `algorithm` header.


================
Comment at: libcxx/include/__algorithm/copy.h:73
+  auto __last_base = __unwrapped_range.second;
+  auto __result_un = std::__unwrap_iter(std::move(__result).base());
 
----------------
Can we use fewer `auto`s here. It's quite hard to understand the types without looking at other places.


================
Comment at: libcxx/include/__algorithm/copy.h:77
   std::__copy_impl(__last_base, __first_base, __result_first);
   return std::make_pair(__last, reverse_iterator<_OutIter>(std::__rewrap_iter(__result_un.second, __result_first)));
 }
----------------
`__last` has been moved from at line 70. It there a test that fails, if not please add a test.


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:30
+struct __unwrap_range_impl {
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __unwrap(_Iter __first, _Sent __sent)
+    requires random_access_iterator<_Iter> && sized_sentinel_for<_Sent, _Iter>
----------------
Please don't use the automatically deduced `auto`.


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:34
+    auto __last = ranges::next(__first, __sent);
+    return pair{std::__unwrap_iter(std::move(__first)), std::__unwrap_iter(std::move(__last)).first};
+  }
----------------
Based on the code in `__algorithm/copy.h` with all `.first.first` and the like. I would prefer to use a small struct with good names for the types instead of a `pair`. I think that will aid the readability of the code in `__algorithm/copy.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129040