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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/copy.h:13
 #include <__algorithm/unwrap_iter.h>
+#include <__algorithm/unwrap_range.h>
 #include <__config>
----------------
Mordante wrote:
> Please also add this to the top level `algorithm` header.
Why should we do that? It's an implementation detail, and adding the include to the top level header just exposes that implementation detail (even for modules users).


================
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)));
 }
----------------
Mordante wrote:
> `__last` has been moved from at line 70. It there a test that fails, if not please add a test.
See D129044.


================
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>
----------------
Mordante wrote:
> Please don't use the automatically deduced `auto`.
I can see you point about readability in general, but in this case I don't think it helps in any way. It would essentially be `pair<decltype(std::__unwrap_iter(std::move(std::declval<_Iter>()))), decltype(std::__unwrap_iter(std::move(std::declval<_Iter>())).first)>` which is just a really long way to write "Find it out yourself". (The `std::move`s might be unnecessary, but removing them wouldn't make it any better)


================
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};
+  }
----------------
Mordante wrote:
> 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`.
I've got
```
template <class _OrigIter, class _Iter>
struct _UnwrappedRange {
  _OrigIter __rewrap;
  _Iter __first;
  _Iter __last;
};
```
right now, but that loses the information that you can only rewrap `__first`. That might be acceptable, Idk. I also really dislike `__rewrap`. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129040