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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks for the patch! I have a few comments that would ideally require a post-commit change, but apart from that everything LGTM.



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:55
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
   static void advance(_Iter& __iter, _Distance __count) {
     std::advance(__iter, __count);
----------------
Let's not fix this immediately, but we should use `__advance`, `__next`, etc. for all these names. I understand the reasoning that those are "reserved names" because they are used elsewhere in the library, however this is playing with fire. For example, `std::next` was introduced in C++11. Hence, users could technically `#define next(...)` in C++03 and use `<algorithm>` in a valid program. Right now, this would break since we are using the `next` name. To avoid having to even think about this, I would just throw `__` in front of anything that's internal, period. It's less pretty, but it's simpler and more robust.


================
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);
----------------
Can we add a test specifically for this bug?


================
Comment at: libcxx/include/__algorithm/sort.h:174
+template <class _AlgPolicy, class _Compare, class _ForwardIterator>
+_LIBCPP_HIDDEN unsigned __sort5_wrap_policy(
+    _ForwardIterator __x1, _ForwardIterator __x2, _ForwardIterator __x3, _ForwardIterator __x4, _ForwardIterator __x5,
----------------
`_LIBCPP_HIDE_FROM_ABI`?


================
Comment at: libcxx/include/__algorithm/sort.h:616
   return __log2;
 }
 
----------------
Here is another approach that would maintain ABI compatibility, but in which nobody writing new code would actually use the explicitly instantiated functions in the dylib. I think it would be reasonable to investigate making this simplification after the LLVM 15 release is done (we should compile significant code bases and try to see if we notice regressions, and if not, we could do this *and* include the `__sort` instantiations only in the stable ABI).

```
//
// In this header:
//
template <class _AlgPolicy, class _RandomAccessIterator, class _Comp>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
void __sort_impl(_RandomAccessIterator __first, _RandomAccessIterator __last, _Comp& __comp) {
  std::__debug_randomize_range<_AlgPolicy>(__first, __last);

  using _Comp_ref = typename __comp_ref_type<_Comp>::type;
  if (__libcpp_is_constant_evaluated()) {
    std::__partial_sort<_AlgPolicy, _Comp_ref>(__first, __last, __last, _Comp_ref(__comp));

  } else {
    typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
    difference_type __depth_limit = 2 * __log2i(__last - __first);
    std::__introsort<_AlgPolicy, _Compare>(__first, __last, __comp, __depth_limit);    
  }
}

//
// In algorithm.cpp:
//

// Only used for STL classic algorithms. We keep this name for ABI compatibility because we have
// been explicitly instantiating `__sort` functions in the shared library.
template <class _Compare, class _RandomAccessIterator>
void __sort(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
  std::__sort_impl<_ClassicAlgPolicy>(std::move(__first), std::move(__last), std::move(__comp));
}

// All the explicit instantiations
[...]
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823