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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with CI fixed and my comments applied.



================
Comment at: libcxx/include/__algorithm/ranges_clamp.h:42
+                          _Proj __proj = {}) const {
+    _LIBCPP_ASSERT(!bool(std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __low))),
+                   "Bad bounds passed to std::ranges::clamp");
----------------
Can we add a test with a predicate that returns something explicitly convertible to `bool`? This should only be supported in the Ranges version.


================
Comment at: libcxx/include/__algorithm/ranges_clamp.h:45-51
+    auto&& __val = std::invoke(__proj, __value);
+    if (std::invoke(__comp, std::forward<decltype(__val)>(__val), std::invoke(__proj, __low)))
+      return __low;
+    else if (std::invoke(__comp, std::invoke(__proj, __high), std::forward<decltype(__val)>(__val)))
+      return __high;
+    else
+      return __value;
----------------
Based on our discussion just now, your value category test is correct, however this optimization is wrong because it can lead to double-moves.

It would be possible to catch this with:

```
// large values to avoid SSO
int val = 100000000000000000000000000000000;
int low = 200000000000000000000000000000000;
int high = 300000000000000000000000000000000;
auto proj = [](int i) { return std::to_string(i); }; // important: returns a temporary
auto comp = [](std::string lhs, std::string rhs) { // important: moves out of rvalue arguments
    return std::atoi(lhs.c_str()) < std::atoi(rhs.c_str());
};
std::ranges::clamp(val, low, high, comp, proj);
```

Something like this should trigger a double-move. Probably easier to test with a Tracked value of some sort.

Also, after adding the test, can you make sure that your current implementation does fail?



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp:9
+
+// <algorithm>
+
----------------
I would XFAIL this on `apple-clang-13`. That's the CI issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126193