[libcxx-commits] [PATCH] D119589: [libc++][ranges] Implement ranges::min

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_min.h:61
+
+    _LIBCPP_ASSERT(__first != __last, "range has to have at least one element");
+
----------------
var-const wrote:
> Question: the Standard has the requirement that the range is not empty for both this overload and the `initializer_list` overload, but it is only checked here. AFAICT, `__min_element::__fn::__go` doesn't do this check. Is there a reason to only apply this check here?
Nice catch!

Wording nit: `s/has to/must/`

It is difficult, but possible, to create an empty initializer_list:
```
std::initializer_list<int> il = {};
std::ranges::min(il);  // UB
```
I agree let's be consistent: either both should `_LIBCPP_ASSERT` or neither should. I have no particular preference which.

@var-const, both `ranges::min_element` and `std::min_element` are totally fine with a zero-element range; in that case there is no min element found, so they return `last` (which points to no element of the range).


================
Comment at: libcxx/include/__algorithm/ranges_min.h:66
+      if (std::invoke(__comp, std::invoke(__proj, *__first), std::invoke(__proj, __result)))
+        __result = *__first;
+    }
----------------
philnik wrote:
> var-const wrote:
> > Some thoughts: this could lead to a lot of copying in the worst case. `min_element` doesn't have this problem because it stores the iterator instead; however, it's fine for `min_element` operating on `forward_iterator`s but cannot be used here since we're iterating an `input_range`. It probably doesn't matter in most scenarios, but in the worst case, I could imagine that calling `min` on `std::vector<ExpensiveToCopy>` could result in copying every single element if the vector happens to be in descending order.
> > 
> > I wonder if it would be worthwhile to have an `if constexpr` to only copy iterators if the range is a forward range, and fall back to this implementation otherwise?
> I thought about that too, let's see what @Quuxplusone thinks.
Good point! I'm slightly worried there might be a subtlety we're all missing, but let's run with it for now.
However, @philnik, please put an `} else {` around lines 67–72: we don't want to instantiate that part of the template when `forward_range<_Rp>` is true.


================
Comment at: libcxx/include/__ranges/take_view.h:121
       auto __n = ranges::size(__base_);
-      // TODO: use ranges::min here.
-      return std::min(__n, static_cast<decltype(__n)>(__count_));
+      return ranges::min(__n, static_cast<decltype(__n)>(__count_));
     }
----------------
philnik wrote:
> var-const wrote:
> > Thanks for going back to fix this! Sorry about the annoying comment -- do you know if this change affects the visible behavior of `take_view` (e.g., some code compiles now that wouldn't compile previously, or vice versa)? If yes, could you please add a test(s) for this?
> I don't think so, but I could be wrong.
This is just taking the min of two integers both of type `range_size_t<_View>`. I don't think there's any possible way for the behavior to be different.
In fact, if I ran the zoo, we'd just do
```
using _SizeT = range_size_t<_View>;
_SizeT __n = ranges::size(__base_);
return __n < static_cast<_SizeT>(__count_) ? __n : static_cast<_SizeT>(__count_);
```
and not pay for the instantiation (or inclusion) of `ranges::min` at all. (But generally libc++ style is to follow the reference implementation to the letter, so as to avoid bugs due to exactly the kind of over-cleverness I'm displaying here. So I recommend keeping this as ranges::min.)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:122
+    assert(ret == 5);
+  }
+  { // test comparator
----------------
var-const wrote:
> Nit: consider adding newlines between braced scopes. I just think it makes it easier to separate different test cases visually.
Ultranit: I have a slight preference for //not// newlines between braced scopes — two almost-empty lines is enough separation for me. However, I also would change line 88 from
```
  { // test comparator
```
to
```
  {
    // test comparator
```
(and likewise line 82, 129, and anywhere else I missed). Please make the line 82/88/etc change; and I //recommend// re-removing the blank lines, but won't die on that hill. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589