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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:57-58
+  ranges::minmax_result<range_value_t<_Rp>> operator()(_Rp&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    auto __iters = __minmax_element::__fn::__go(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+    return ranges::minmax_result<range_value_t<_Rp>> { *__iters.min, *__iters.max };
+  }
----------------
Quuxplusone wrote:
> Sadly, I don't think we can do this. `_Rp` is an `input_range`, which means it's single-pass. You can't pass its iterators to `ranges::minmax_result`. You've sidestepped the guardrail by calling into `__go` directly instead of the constrained `minmax_result::operator()` — and that's fine with me — but it means that what you've got here //will// compile but //won't// work.
> 
> We should add a regression test for this. Perhaps using a completely custom `input_iterator`. I've thought of two test cases that use only STL facilities, but libc++ has not implemented either `istream_view` or `move_iterator` yet. Also, my `move_iterator` example is possibly wrong. See https://godbolt.org/z/7Kov5e6oe (and I've asked on Slack why libstdc++ barfs on the latter).
Would it make sense to change input iterators in `test_iterators.h` to throw if they're read more than once?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637