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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_min.h:66
+      if (std::invoke(__comp, std::invoke(__proj, *__first), std::invoke(__proj, __result)))
+        __result = *__first;
+    }
----------------
Quuxplusone wrote:
> 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.
According to https://godbolt.org/z/fjaxc9rT5 it doesn't get instantiated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589