[libcxx-commits] [PATCH] D120637: [libc++][ranges] Implement ranges::minmax and ranges::minmax_element

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:56
+
+  template <input_range _Rp, class _Proj = identity,
+            indirect_strict_weak_order<projected<iterator_t<_Rp>, _Proj>> _Comp = ranges::less>
----------------
Could you investigate whether the logic of this algorithm can be shared with the STL classic `std::minmax_element`? IMO this is right on the threshold where it starts making sense to share the implementation.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:84
+  ranges::minmax_result<_Ip> operator()(_Ip __first, _Sp __last, _Comp __comp = {}, _Proj __proj = {}) const {
+    return __go(__first, __last, __comp, __proj);
+  }
----------------
philnik wrote:
> Quuxplusone wrote:
> > var-const wrote:
> > > philnik wrote:
> > > > var-const wrote:
> > > > > Would it make sense to move `__first` and maybe `__last`?
> > > > iterators should //always// be cheap to copy, and `ranges::minmax_element` doesn't support input ranges. So I see no reason to move them.
> > > I don't feel very strongly, but I don't think we should rely on users doing something that isn't enforced. Elsewhere we're protecting against things that are pretty unreasonable just because they're allowed. It's a good point that an iterator that's expensive to copy is more likely to be an input iterator, though.
> > FWIW, if we really thought iterators/sentinels might be //expensive//, we might even choose to pass them by reference instead of by value. (In this specific case, we could take `const _Sp&`.) We're just guessing that `_Sp` is likely more appropriate than `const _Sp&` (because //usually// `_Sp` is either a pointer/iterator, or a tag type). I would //not// pass-by-reference just to avoid a copy of an iterator or sentinel. (But I would pass-by-reference to avoid copies of callables. Inconsistent, yes. :))
> > 
> > Also, the big downside of adding std::moves has already been illustrated, by me, literally yesterday: in D118003 I wrote a std::move that shouldn't have been there, and thus introduced a use-after-move bug. We have no good story for testing against use-after-move of iterators and sentinels. So let's be cautious in this department.
> > 
> > However, just changing `__first` into `std::move(__first)` sounds like a pretty harmless improvement to me. It //might// be faster and it will //never// be slower. So +1 on std::move'ing from me.
> > I do see how it's a slippery slope, because our STL Classic algorithms generally do //not// std::move iterators, and I don't think we should actually go churn up that existing code to add moves. But in new code, and specifically Ranges code where we might expect slightly-heavier-weight iterators, and //specifically// in this case where the function is one line long — I think we can justify std::move!
> > 
> > (But I'll defer to @ldionne if he has an opinion.)
> Agreed that it doesn't hurt here, but I definitely wouldn't make that a guarantee. 
I'm neutral. We can do it for one-liners, but we shouldn't start trying to move out of iterators whenever we can, since the benefit is likely small. It's more important that users have the mental model that iterators and sentinels must be cheap to copy, because otherwise nothing's going to work well, even if we try really hard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637