[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/iterator_operations.h:50
+template <>
+struct _Algs<_RangeAlgPolicy> {
+  static constexpr auto min_element = ranges::min_element;
----------------
philnik wrote:
> 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?
Sorry, looks like I messed up merging. Should be good now, thanks for checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823