[libcxx-commits] [PATCH] D142864: [libc++] `<algorithm>`: `ranges::minmax` should dereference iterators only once

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:129-131
+    _LIBCPP_ASSERT(forward_iterator<_It>, "_It must be forward_iterator");
+    _LIBCPP_ASSERT((sentinel_for<_Se, _It>), "_Se must be sentinel for _It");
+    _LIBCPP_ASSERT((indirect_strict_weak_order<_Pr, projected<_It, _Pj>>), "");
----------------
These should either be `static_assert`s or simply not exist at all.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:133
+
+    _LIBCPP_ASSERT(__first != __last, "range has to contain at least one element");
+
----------------
This has been asserted already, there is no need to check it twice.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:135
+
+    using _ValueT = __iter_value_type<_It>;
+
----------------
I think this should use `iter_value_t`.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:140
+      // This initialization is correct, similar to the N4928 [dcl.init.aggr]/6 example
+      minmax_result<_ValueT> __result = {static_cast<_ValueT>(*__found_min), __result.min};
+      return __result;
----------------
What are these `static_cast`s for?


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:149
+
+    while (++__first != __last) { // process one or two elements
+      _It __prev = __first;
----------------
Why are you effectively re-implementing `minmax_element` here?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:337
 
+class input_move_iterator {
+public:
----------------
Couldn't you use a `cpp20_input_iterator<std::move_iterator<std::string>>`? That would make this test constexpr-compatible and avoid implementing another iterator.


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

https://reviews.llvm.org/D142864