[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;
+    }
----------------
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.


================
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_));
     }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589