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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Please ping me on Discord once you have implemented all comments just so I can do another quick pass, and we can ship this in time for LLVM 15.



================
Comment at: libcxx/.clang-format:71
 
-PenaltyIndentedWhitespace: 61
+PenaltyIndentedWhitespace: 2
 
----------------
Shouldn't be here.


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:45
+  {
+    return std::__rewrap_iter(__orig_iter, __iter);
+  }
----------------
`std::move`?


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:48
+
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __rewrap(_Iter, _Iter __iter) { return __iter; }
+};
----------------
This would avoid a copy?


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:53
+struct __unwrap_range_impl<_Iter, _Iter> {
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __unwrap(_Iter __first, _Iter __last) {
+    return pair{std::__unwrap_iter(std::move(__first)), std::__unwrap_iter(std::move(__last))};
----------------
Please leave at least some sort of documentation explaining what this file and group of functions do. And in particular, it would be useful to explain why they are useful/needed.


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:59
+  __rewrap(_Iter __orig_iter, decltype(std::__unwrap_iter(__orig_iter)) __iter) {
+    return std::__rewrap_iter(__orig_iter, __iter);
+  }
----------------
Would it make sense to `std::move` here?


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:74
+}
+#else
+template <class _Iter, class _Unwrapped = decltype(std::__unwrap_iter(std::declval<_Iter>()))>
----------------
I had completely missed this because it's a large-ish block.


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:77
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR pair<_Unwrapped, _Unwrapped> __unwrap_range(_Iter __first, _Iter __last) {
+  return std::make_pair(std::__unwrap_iter(__first), std::__unwrap_iter(__last));
+}
----------------
`std::move`


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:82
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Iter __rewrap_range(_Iter __orig_iter, _Unwrapped __iter) {
+  return std::__rewrap_iter(__orig_iter, __iter);
+}
----------------
`std::move`


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:335-339
-  static _LIBCPP_CONSTEXPR decltype(std::__unwrap_iter(std::declval<_Iter>()))
-  __apply(_ReverseWrapper<_Iter> __i) _NOEXCEPT {
-    return std::__unwrap_iter(__i.base().base());
-  }
-};
----------------
I think this diff doesn't belong here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129040