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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__algorithm/copy.h:13
 #include <__algorithm/unwrap_iter.h>
+#include <__algorithm/unwrap_range.h>
 #include <__config>
----------------
philnik wrote:
> 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).
Are other header not allowed to use this? `unwrap_iter` is also in `<algorithm>` so IMO this should be there too.


================
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>
----------------
philnik wrote:
> 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)
I agree that's also not nice to read. But I think we should give some thought about whether we can do better. I feel this entire unwrapping looks quite hard to understand. The original unwrap code already took some effort to understand, but I feel it gets worse with ranges.

I will give it some more thought.


================
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};
+  }
----------------
philnik wrote:
> 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?
Maybe rename `__rewrap` to `__base`?
When it's not possible to rewrap `__last` we can add some comment why it's not possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129040