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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added inline comments.
This revision now requires changes to proceed.


================
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),
----------------
var-const wrote:
> @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?
Since D128864 fixes this properly I would prefer if you revert the changes here and I enable the additional tests over there instead.


================
Comment at: libcxx/include/__algorithm/inplace_merge.h:62
 {
+    using _Ops = _IterOps<_AlgPolicy>;
+
----------------
Why are you adding an alias here? It doesn't look like much of a benefit for two calls. Ditto for a lot of other places.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:50
+template <>
+struct _Algs<_RangeAlgPolicy> {
+  static constexpr auto min_element = ranges::min_element;
----------------
var-const wrote:
> Should this be moved to a different file? Or perhaps this file should be renamed from `iterator_operations.h` to something more generic?
I'm already not very happy that we pull all the iterator operations in. We should avoid pulling whole algorithms in. Instead, I think the better option would be to rewrite `min_element` to support ranges like that:
```
template <class _Iter, class _Sent, class _Proj, class _Comp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
_Iter __min_element(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) {
  if (__first == __last)
    return __first;

  _Iter __i = __first;
  while (++__i != __last)
    if (std::__invoke(__comp, std::__invoke(__proj, *__i), std::__invoke(__proj, *__first)))
      __first = __i;
  return __first;
}

template <class _ForwardIterator, class _Compare>
_LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 _ForwardIterator
min_element(_ForwardIterator __first, _ForwardIterator __last, _Compare __comp)
{
  static_assert(__is_callable<_Compare, decltype(*__first), decltype(*__first)>::value,
              "The comparator has to be callable");
  auto __proj = __identity();
  return std::__min_element(__first, __last, __comp, __proj);
}
```
and update `libcxx/algorithms/callable.verify.cpp`.


================
Comment at: libcxx/include/__algorithm/sort.h:142
+                                is_arithmetic<_Tp>::value && __is_simple_comparator<_Compare>::value &&
+                                is_same<decltype(*declval<_Iter>()), _Tp>::value>;
 
----------------
Shouldn't this be `is_same<decltype(*declval<_Iter>()), _Tp&>::value`? Otherwise this would disable the optimization for (almost) all iterators, especially raw pointers. Also, could you qualify `declval`? While it's not strictly necessary it makes error messages a lot better.


================
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)
----------------
var-const wrote:
> 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`).
As I already said above, I think the better alternative is to make `__min_element` work with both classic and ranges iterators.


================
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
----------------
var-const wrote:
> @ldionne This gives me some concern. Do these changes look reasonable to you? If yes, should we also add specializations for `_RangeAlgPolicy`?
I'm quite certain that this is an ABI break, since template parameters are mangled. I think you have to revert these changes and add a `__sort_policy` or something like that as a dispatch function. Regarding `_RangeAlgPolicy` I think we should wait a few releases and try things out in the unstable ABI before effectively killing the possibility of changing the ABI in any way (assuming we want to do that at all).


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
IIUC some types of problems are only obvious when looking at the output of the algorithms, so I think it's not enough to ensure that the algorithms can be instantiated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823