[libcxx-commits] [PATCH] D132505: [libc++] Refactor deque::iterator algorithm optimizations

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

I found this approach "having a boolean + some operations" less clearer than defining a concept (or pseudo concept with constexpr bool + sfinae) with several customization points.



================
Comment at: libcxx/include/__algorithm/copy.h:33
+          __enable_if_t<!(is_copy_constructible<_InIter>::value
+                       && is_copy_constructible<_Sent>::value
+                       && is_copy_constructible<_OutIter>::value), int> = 0>
----------------
sentinel is always copyable.
this applies for other places


================
Comment at: libcxx/include/__algorithm/copy.h:74
+      auto __iters = std::__copy(__first, __last, __segment.__begin_);
+      __result += __range_size;
+      return std::make_pair(__iters.first, __result);
----------------
`+=` isn't required as part of your "concept" of segmented iterator


================
Comment at: libcxx/include/__algorithm/copy.h:86
+          class _OutIter,
+          __enable_if_t<__segmented_iterator_traits<_InIter>::__is_segmented_iterator::value, int> = 0>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_InIter, _OutIter>
----------------
As we get more and more optimisations for different types, it is harder to make sure all of these overloads are mutually exclusive. Do you think this is (or will be) a problem?


================
Comment at: libcxx/include/__iterator/segmented_iterator.h:17-25
+// - `static <undefined> __get_forward_range(_Iterator __first, _Iterator __last);`
+//   where <undefined> is a range which contains the segments to iterate over. The segments are iterated front to back.
+// - `static <undefined> __get_backward_range(_Iterator __first, _Iterator __last);`
+//   where <undefined> is a range which contains the segments to iterate over. The segments are iterated back to front.
+// - `static <undefined> __get_range_to_segment_begin(_Iterator __end)`
+//   where <undefined> is a struct where [__begin_, __end_) is a range from begin of the segment to
+//   the passed-in iterator
----------------
It might be a good idea to put these functions in the primary template `__segmented_iterator_traits` and `=delete` them. This way it is more self documentary. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132505