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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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");
----------------
ldionne wrote:
> Can we add a test with a predicate that returns something explicitly convertible to `bool`? This should only be supported in the Ranges version.
Hmm, it looks like the algorithm constraints actually prevent a return type that's only explicitly (and not implicitly) convertible to `bool`:
- the algorithm requires `indirect_strict_weak_order` for `_Comp`;
- `indirect_strict_weak_order` [requires](https://eel.is/c++draft/indirectcallable.indirectinvocable#concept:indirect_strict_weak_order) `strict_weak_order`;
- `strict_weak_order` requires `relation`;
- `relation` requires `boolean-testable`;
- `boolean-testable` requires `boolean-testable-impl`;
- `boolean-testable-impl` [requires](https://eel.is/c++draft/concept.booleantestable#concept:boolean-testable-impl) `std::convertible_to<T, bool>`;
- `std::convertible_to<T, bool>` requires *both* an explicit and an implicit conversion.

If I'm reading this correctly, the `indirect_strict_weak_order` constraint prevents the comparator that returns a type explicitly convertible to `bool`. If so, the `bool` casts in the definition of `clamp` in the standard appear redundant (at least for the range algorithm -- since the same wording is used for the classic algorithm as well, perhaps it's there because of it?). What do you think -- perhaps I'm missing something?


================
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;
----------------
ldionne wrote:
> 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?
> 
Added a new test case and verified it fails for the original version (I used a simple struct that checks for double-moving).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp:9
+
+// <algorithm>
+
----------------
ldionne wrote:
> I would XFAIL this on `apple-clang-13`. That's the CI issue.
Right -- it looks like this has already come up before in `test/std/ranges/range.adaptors/range.filter/pred.pass.cpp` which has the following comment:
```
// Older Clangs don't properly deduce decltype(auto) with a concept constraint
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126193