Details
- Reviewers
ldionne Mordante philnik - Group Reviewers
Restricted Project - Commits
- rGa203acb9dd72: [libc++][ranges] Implement `ranges::clamp`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/docs/Status/RangesAlgorithms.csv | ||
---|---|---|
29–30 | Please don't forget to update the link before merging. | |
libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp | ||
24 | Can you also check SFINAE with indirect_strict_weak_order? | |
33 | 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. | |
39 | Can you also check with a comparator that returns something convertible to bool? | |
41 | 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? |
libcxx/include/algorithm | ||
---|---|---|
290 | Please don't forget to remove this merge conflict marker. |
libcxx/include/algorithm | ||
---|---|---|
290 | Thanks! |
- Fix a bug where the result of applying a projection could lose its value category and add a test;
- fix incorrect assertion checking that low <= high and add an assertion test for it;
- add SFINAE tests;
- add ranges::clamp to the relevant robust test suites;
- address the existing feedback.
LGTM with CI fixed and my comments applied.
libcxx/include/__algorithm/ranges_clamp.h | ||
---|---|---|
43 | Can we add a test with a predicate that returns something explicitly convertible to bool? This should only be supported in the Ranges version. | |
46–52 | 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? | |
libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp | ||
10 | I would XFAIL this on apple-clang-13. That's the CI issue. |
Rebase on main.
libcxx/include/__algorithm/ranges_clamp.h | ||
---|---|---|
43 | Hmm, it looks like the algorithm constraints actually prevent a return type that's only explicitly (and not implicitly) convertible to bool:
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? | |
46–52 | Added a new test case and verified it fails for the original version (I used a simple struct that checks for double-moving). | |
libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp | ||
10 | 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 |
Please don't forget to update the link before merging.