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

Mon Apr 4 07:51:03 PDT 2022

ldionne accepted this revision.
ldionne added a comment.

LGTM with my comments applied. Thanks!

================
Comment at: libcxx/include/__algorithm/minmax.h:49
+pair<_Tp, _Tp> minmax(initializer_list<_Tp> __t, _Compare __comp) {
+    __identity __proj;
+    auto __ret = std::__minmax_element_impl(__t.begin(), __t.end(), __comp, __proj);
----------------
Since we start using `std::invoke` under the hood, this means that we would suddenly start accepting non-function-objects too as a comparator in STL classic algorithms. I don't think we want to relax the requirements like this, since it could be a portability trap for users.

Can we start `static_assert`ing that the comparator can be called like `__comp(...)` in the STL algorithms?

================
Comment at: libcxx/include/__algorithm/minmax_element.h:37
+  bool operator()(_Iter& __it1, _Iter& __it2) {
+    return std::__invoke_constexpr(__comp,
+                                   std::__invoke_constexpr(__proj, *__it1),
----------------
You just said during live review: I don't think I have a test to check that I'm using `std::invoke` for the comparator. Please add one!

================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:65
+
+    ranges::minmax_result<_Ip> __result = {*__first, *__first};
+    if (__first == __last || ++__first == __last)
----------------
philnik wrote:
> ldionne wrote:
> > 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.
> I think http://eel.is/c++draft/dcl.init.aggr#7 is the line in question here, and that sounds to me like the first element has to be fully initialized before the second one.
http://eel.is/c++draft/dcl.init.aggr#7 says:

> The initializations of the elements of the aggregate are evaluated in the element order. That is, all value computations and side effects associated with a given element are sequenced before those of any element that follows it in order.

I am on the fence about whether this means what you think it means. In particular, if the "initializations of the elements of the aggregate are evaluated in the element order" means that the object isn't considered partially formed before the semi-colon, then the status quo works. I'll ask the question on a reflector, but until we have an answer, I think it would be safer to use this, even if it results in an additional copy:

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

Just to unblock this review -- I'll send an email later today.

================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:60
+
+  while (++__first != __last) {
+    _Iter __i = __first;
----------------
philnik wrote:
> ldionne wrote:
> > 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
> https://quick-bench.com/q/ezc6hrE4rQ4Smt-XXF72ENAx3iI
> It looks like using `[[unlikely]]` makes no difference and checking the size before the loop is slightly slower.
Alright, in that case I don't mind leaving the code as-is, but thanks for checking. Would you mind adding your simple benchmark code to `libcxx/benchmarks/algorithms.bench.cpp`?

================
Comment at: libcxx/include/type_traits:419-420
 */
-
-#include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
----------------
This looks like an incorrect merge conflict resolution to me.

================
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);
----------------
ldionne wrote:
> 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.
You're still using a comparator that isn't a strict-weak order. I think you can probably replace this by my suggestion above, which checks the same property without being IFNDR.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637