[libcxx-commits] [PATCH] D129823: [libc++][ranges] Make range algorithms support proxy iterators

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:31
+  auto __last_iter = _IterOps<_AlgPolicy>::next(__first, __last);
+  auto __ret = std::__copy(reverse_iterator<_Iter1>(__last_iter),
+                           reverse_iterator<_Iter1>(__first),
----------------
@philnik I think this was a bug; it wasn't caught because `copy_backward` tests never try the case where the sentinel isn't the same type as the iterator. This fix is enough to make `robust_against_proxy_iterators` pass; however, if I change `test_iterators` in `copy_backward` test to define `class Sent` as `= sentinel_wrapper<In>`, I get more breakages. Could you please look into this?


================
Comment at: libcxx/include/__algorithm/inplace_merge.h:2
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
These changes are simple but very intrusive. The vast majority is replacing unqualified calls to `swap` with `_IterOps<_AlgPolicy>::iter_swap`, and calls to `*i1 = std::move(*i2)` with `_IterOps<_AlgPolicy>::__iter_move(i2)`.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:50
+template <>
+struct _Algs<_RangeAlgPolicy> {
+  static constexpr auto min_element = ranges::min_element;
----------------
Should this be moved to a different file? Or perhaps this file should be renamed from `iterator_operations.h` to something more generic?


================
Comment at: libcxx/include/__algorithm/ranges_fill.h:33
   _Iter operator()(_Iter __first, _Sent __last, const _Type& __value) const {
-    if constexpr(random_access_iterator<_Iter>) {
+    if constexpr(random_access_iterator<_Iter> && sized_sentinel_for<_Sent, _Iter>) {
       return ranges::fill_n(__first, __last - __first, __value);
----------------
@philnik I think this was a bug -- please let me know if I'm missing something, or if you would prefer a different fix.


================
Comment at: libcxx/include/__algorithm/sort.h:226
   for (--__lm1; __first != __lm1; ++__first) {
-    _BidirectionalIterator __i = _VSTD::min_element(__first, __last, __comp);
+    _BidirectionalIterator __i = _Algs<_AlgPolicy>::min_element(__first, __last, __comp);
     if (__i != __first)
----------------
This is one case where the difference between ranges and classic versions isn't confined to iterator operations (but `stable_sort` would require a similar call to `rotate`).


================
Comment at: libcxx/src/algorithm.cpp:16
 
-template void __sort<__less<char>&, char*>(char*, char*, __less<char>&);
+template void __sort<_ClassicAlgPolicy, __less<char>&, char*>(char*, char*, __less<char>&);
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
----------------
@ldionne This gives me some concern. Do these changes look reasonable to you? If yes, should we also add specializations for `_RangeAlgPolicy`?


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.compile.pass.cpp:152
+  //test(std::ranges::prev_permutation, in, binary_pred);
+  //test(std::ranges::next_permutation, in, binary_pred);
+}
----------------
Uninitialized memory algorithms don't work with `ProxyIterator` because it doesn't satisfy `nothrow-input-iterator` that requires the result of dereferencing the iterator to be the same as `iter_reference_t`. I'm not sure if it's possible to write a different valid proxy iterator that would satisfy that constraint but still require special handling in the implementation. @huixie90 What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823