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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Mar 29 07:16:36 PDT 2022


ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks a lot for the patch! I really like the new approach where we avoid duplicating the logic in `minmax_element`, and I think we should try to use this approach for other algorithms when the benefit outweighs the cost of doing so (which will be the case for non-trivial algos, but not for very simple ones).

Once my comments have been addressed, I think we'll be pretty much ready to ship this, but I'd still like to have a look to confirm. Thanks!



================
Comment at: libcxx/include/__algorithm/minmax.h:13
 #include <__algorithm/comp.h>
+#include <__algorithm/ranges_minmax_element.h>
 #include <__config>
----------------
We'll want to put the common logic in the classic header, not in the ranges header. The classic version shouldn't have a dependency on the ranges version. It might be a bit counter-intuitive because the generalized algorithm looks more like a ranges algorithm, but it's necessary if we want to avoid a dependency on ranges.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:51
+  ranges::minmax_result<_Type> operator()(initializer_list<_Type> __il, _Comp __comp = {}, _Proj __proj = {}) const {
+    _LIBCPP_ASSERT(__il.begin() != __il.end(), "initilaizer_list has to contain at least one element");
+    auto __iters = std::__minmax_element_impl(__il.begin(), __il.end(), __comp, __proj);
----------------



================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:70
+      return {*__result.first, *__result.second};
+    } else {
+      auto __less = [&](auto&& __a, auto&& __b) -> bool {
----------------
Could you add a short comment in the lines of `// We can't copy input_iterators, so we need to use a different implementation based on stashing their value_type instead`.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:71
+    } else {
+      auto __less = [&](auto&& __a, auto&& __b) -> bool {
+        return std::invoke(__comp, std::invoke(__proj, std::forward<decltype(__a)>(__a)),
----------------
Suggestion if you don't like the `decltype(__a)` below:

```
auto __less = [&]<class _Vp>(_Vp&& __a, _Vp&& __b) -> bool { ... std::forward<_Vp>(__a) ... };
```


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:65
+
+    ranges::minmax_result<_Ip> __result = {*__first, *__first};
+    if (__first == __last || ++__first == __last)
----------------
Quuxplusone wrote:
> Per @BRevzin https://cpplang.slack.com/archives/C4Q3A3XB8/p1646101005063689 , I think this needs to be
> ```
> ranges::minmax_result<_Ip> __result = {*first, __result.min};
> ```
> because it might not be safe to copy from `*first` twice. You can regression-test this with something like
> ```
> std::shared_ptr<int> a[] = { std::make_shared<int>(42) };
> auto [min, max] = std::ranges::minmax(std::move_iterator(a), std::move_iterator(a + 1));
> assert(a[0] == nullptr);
> assert(min != nullptr);
> assert(max == min);
> ```
I don't *think* `__result` is considered initialized until the semicolon, though, which means that we'd be accessing an object before it has been initialized, which is UB. Why can't we do this instead:

```
auto __tmp = *__first;
ranges::minmax_result<_Ip> __result = {__tmp, std::move(__tmp)};
```

I asked a question in the thread, waiting for an answer.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:37
+pair<_Iter, _Iter> __minmax_element_impl(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) {
+  auto __less = [&](_Iter& __it1, _Iter& __it2) -> bool {
+    return std::__invoke(__comp, std::__invoke(__proj, *__it1), std::__invoke(__proj, *__it2));
----------------
This will need to be made C++03 friendly, so unfortunately you'll need to use function objects and drop the nice syntax. :(

This may mean that it's not worth refactoring the small `if` bits in lambdas below.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:60
+
+  while (++__first != __last) {
+    _Iter __i = __first;
----------------
Per our discussion: it would be interesting to check which of the following options is best in terms of codegen:

1. Use `[[unlikely]]` on the `if (++__first == __last)` branch to tell the compiler that we rarely hit the end of the range, or
2. If we have random-access iterators, handle the case of an odd-sized range explicitly above so we can get rid of the `if (++__first == __last)` check in the loop altogether and always process two elements at a time, or
3. Leave the code as-is


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:87
+template <class _Tp>
+using minmax_result = min_max_result<_Tp>;
+
----------------
I think this should be `minmax_element_result`, and so we're missing a `minmax_result` alias in `ranges_minmax.h`. Please add tests for both!


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:94
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  ranges::minmax_result<_Ip> operator()(_Ip __first, _Sp __last, _Comp __comp = {}, _Proj __proj = {}) const {
+    auto __ret = std::__minmax_element_impl(std::move(__first), std::move(__last), __comp, __proj);
----------------
This should be returning `mimmax_element_result` instead. Even though it's not observable, let's do it for tidyness. Same below.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:102
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  ranges::minmax_result<borrowed_iterator_t<_Rp>> operator()(_Rp&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    auto __ret = std::__minmax_element_impl(ranges::begin(__r), ranges::end(__r), __comp, __proj);
----------------
It looks like the Standard gets confused between `minmax_result` and `minmax_element_result`. In the description of `minmax_element` (just above http://eel.is/c++draft/alg.min.max#31), it uses `minmax_result` as the return type of the algorithm. However, it uses `minmax_element_result` in the synopsis of the same algorithms (in http://eel.is/c++draft/algorithm.syn).

I'm sure this is editorial and they meant to use `minmax_element_result`. Can you please file a LWG issue about this? You can read how to do this near the top of https://cplusplus.github.io/LWG/lwg-active.html. Please CC me on the thread.

As far as this patch is concerned, please make sure that `minmax_element` is described to return `minmax_element_result`:
1. In the synopsis
2. In the code
3. In the test synopsis
4. In the test code for `minmax_element` itself


================
Comment at: libcxx/include/algorithm:118
+    indirect_strict_weak_order<projected<I, Proj>> Comp = ranges::less>
+  constexpr I minmax_element(I first, S last, Comp comp = {}, Proj proj = {});            // since C++20
+
----------------
This applies elsewhere too, e.g. below but probably in the tests.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think you should also add an entry in `robust_against_comparator_copying.pass.cpp` or whatever!


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:51-52
+
+static_assert(HasMinMax<int (&)[10]>);
+static_assert(HasMinMax<int (&&)[10]>);
+static_assert(!HasMinMax<NoLessThanOp (&)[10]>);
----------------
Instead of just checking that those are well-formed, can you please add runtime tests below on these types?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:57
+
+static_assert(HasMinMax<std::initializer_list<int>>);
+static_assert(!HasMinMax<std::initializer_list<NoLessThanOp>>);
----------------
Maybe?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:95
+    S a[3] = {S{2}, S{1}, S{3}};
+    decltype(auto) ret = std::ranges::minmax(a[0], a[1], {}, &S::i);
+    ASSERT_SAME_TYPE(decltype(ret), std::ranges::minmax_result<const S&>);
----------------
I think this is actually clearer, since you're making a copy of the `minmax_result`, and the `minmax_result` itself contains a reference. This may apply elsewhere.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:103-105
+    auto r = std::ranges::minmax(one, two, [](int, int) { return false; });
+    assert(r.min == 1);
+    assert(r.max == 2);
----------------
It looks like this is testing http://eel.is/c++draft/alg.min.max#18, please add a comment like `make sure that we return {a, b} if b is not less than a`.

You can then test this by:

```
int a = 1;
int b = 1;
auto r = ranges::minmax(a, b);
assert(&r.min == &a);
assert(&r.max == &b);
```

The current implementation of the test unfortunately doesn't use a strict-weak order, and so technically is IFNDR.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:157
+  {
+    // check that a single element works
+    auto ret = std::ranges::minmax({ 1 });
----------------
Can you please add a test to make sure that we trigger the `_LIBCPP_ASSERT` when we pass an empty `initializer_list`? You can do that by looking at what we do in other tests named `assert.*.pass.cpp` in the test suite. It's pretty easy to do, but you'll want to put it in a separate file because it won't run on Windows. I'd like you to check the `_LIBCPP_ASSERT` for `ranges::minmax` that takes a range.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:286
+int main(int, char**) {
+  test();
+  static_assert(test());
----------------
I see you have tests with an `input_range`, but can you also add generic tests with ranges using the other archetypes like `forward_iterator`, `bidirectional_iterator`, etc?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:39-40
+
+static_assert(HasMinMaxElementR<int (&)[10]>);
+static_assert(HasMinMaxElementR<int (&&)[10]>);
+static_assert(!HasMinMaxElementR<NoLessThanOp (&)[10]>);
----------------
Instead of just checking that those are well-formed, can you please add runtime tests below on these types?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:44
+
+static_assert(HasMinMaxElementR<std::initializer_list<int>>);
+static_assert(!HasMinMaxElementR<std::initializer_list<NoLessThanOp>>);
----------------



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:110
+template <class It>
+constexpr bool test() {
+  test<It>({}, 0, 0);
----------------
I find this function name (and the one above) rather confusing, since we have 3 overloads of `test` in this file. Is there any reason why this isn't called `test_iterators`, and why the `test_iterators` function above (one of the first in the file) isn't redundant at this point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637



More information about the libcxx-commits mailing list