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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/copy.h:91-92
   return std::make_pair(
       std::__rewrap_iter(__first_un.second, std::move(__ret.first)),
       std::__rewrap_iter(__result_un.second, std::move(__ret.second)));
 }
----------------
is it ok pass `nullptr` to `__rewrap_iter`. The implementation below seems that `__first_un.second` can be `nullptr`


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:14
+#include <__concepts/constructible.h>
+#include <__concepts/same_as.h>
+#include <__config>
----------------
unused?


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:32
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __unwrap(_Iter __first, _Sent __last)
+    requires random_access_iterator<_Iter> && sized_sentinel_for<_Sent, _Iter> && copy_constructible<_Iter>
+  {
----------------
`random_access_iterator` implies `copy_constructible`
(forward + are copyable)


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:34
+  {
+    auto __sent = ranges::next(__first, __last);
+    return pair{std::__unwrap_iter(std::move(__first)), std::__unwrap_iter(std::move(__sent)).first};
----------------
not sure about the variable name `__sent`. because there is a template parameter called `_Sent`, at the first glance, I thought `__sent` is of type `_Sent` because of the name. but actually, it is of type `_Iter`


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:39
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __unwrap(_Iter __first, _Sent __last) {
+    return pair{pair{std::move(__first), nullptr}, std::move(__last)};
+  }
----------------
I'd be really cautious about `nullptr`, which is comparable with pointers. if any of the logic goes wrong, it could result in a runtime UB. Is it possible to avoid `nullptr`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129040