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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision as: philnik.
philnik added a comment.

LGTM for now with the `min_element` change and green CI. I think there should be a few follow-up PRs to clean some things up though.



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:50
+template <>
+struct _Algs<_RangeAlgPolicy> {
+  static constexpr auto min_element = ranges::min_element;
----------------
var-const wrote:
> philnik wrote:
> > 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`.
> Discussed offline. I slightly prefer the current approach, but not enough to spend too much time discussing this. Went with the approach you suggested.
> 
Am I missing something, or did you not update this part yet?


================
Comment at: libcxx/include/__algorithm/sort.h:34-80
+// Wraps an algorithm policy tag and a comparator in a single struct, used to pass the policy tag around without
+// changing the number of template arguments. This is only used for the "range" policy tag.
+//
+// To create an object of this type, use `_WrapAlgPolicy<T, C>::type` -- see the specialization below for the rationale.
+template <class _PolicyT, class _CompT, class = void>
+struct _WrapAlgPolicy {
+  using type = _WrapAlgPolicy;
----------------
I'll try to make a follow-up PR to remove this, but as I said I'm Ok with it for now.


================
Comment at: libcxx/include/__algorithm/sort.h:55
+struct _WrapAlgPolicy<_PolicyT, _CompT,
+    typename std::enable_if<std::is_same<_PolicyT, _ClassicAlgPolicy>::value>::type> {
+  using type = _CompT;
----------------
`__enable_if_t`?


================
Comment at: libcxx/include/__algorithm/sort.h:73
+struct _UnwrapAlgPolicy<_Wrapped,
+    typename std::enable_if<!std::is_same<typename _Wrapped::_AlgPolicy, int>::value>::type> {
+  using _AlgPolicy = typename _Wrapped::_AlgPolicy;
----------------
`__enable_if_t`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823