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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a subscriber: ldionne.
Quuxplusone added inline comments.


================
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);
+  }
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637