[libcxx-commits] [PATCH] D126193: [libc++] Implement ranges::clamp

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:35
 Read-only,is_heap_until,Not assigned,n/a,Not started
-Read-only,clamp,Not assigned,n/a,Not started
+Read-only,clamp,Nikolas Klauser,` <https://llvm.org/>`_,✅
 Read-only,is_permutation,Not assigned,n/a,Not started
----------------
Please don't forget to update the link before merging.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp:23
+
+constexpr bool test() {
+  { // low < val < high
----------------
Can you also check SFINAE with `indirect_­strict_­weak_­order`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp:32
+  // val < low < high
+  assert(std::ranges::clamp(10, 20, 30) == 20);
+
----------------
Ultranit: maybe use different values in different test cases? I know it sounds funny but I'm slightly concerned that `return 20;` would pass most if not all the tests. :) It might also make it a bit more obvious which value is being returned (high/low/original) when the same value of `20` doesn't have different meanings in different test cases.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp:38
+  // check that the comparator is used
+  assert(std::ranges::clamp(10, 30, 20, std::ranges::greater{}) == 20);
+
----------------
Can you also check with a comparator that returns something convertible to `bool`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp:40
+
+  { // low = val = high
+    int val = 10;
----------------
Can you also add test cases like `(10, 10, 20)` and `(10, 20, 20)`, i.e., when `value` is equal to one of the bounds but not the other?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126193