- checking that the algorithm supports predicates returning a non-boolean type that's implicitly convertible to bool;
- checking that predicates and/or projections are invoked using std::invoke.
Details
- Reviewers
philnik - Group Reviewers
Restricted Project - Commits
- rGd4c53202ebb6: [libc++][ranges][NFC] Consolidate some repetitive range algorithm tests:
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
All in all I think it's a good idea. I don't really see a way to fuck it up other than with malicious intent if it compiles. You'd have to do something like if (!is_same_v<decltype(pred()), bool>) { /*do stuff*/ }). If you do bool b = pred() or if (pred()) it would be implicitly converted to bool. if you do auto x = pred(); auto y = x; if (x) {} it would error at compile-time. IIUC even if you'd use invoke_r<bool, ...>(pred) it should get implicitly converted.
libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.pass.cpp | ||
---|---|---|
19 ↗ | (On Diff #443402) | Nit: Add a newline between <> and "" includes |
77 ↗ | (On Diff #443402) | I don't think a TODO makes a lot of sense here. We are quite good at adding the tests as we go by now I think. |
169–173 ↗ | (On Diff #443402) | Maybe just make it a .compile.pass.cpp? It's not like we learn anything from running them. They probably work properly if they compile. |
My plan is to omit these tests from the remaining unimplemented range algorithms going forward, but leave the existing tests be for now (I'll clean them up later once we're done with the initial implementation of the algorithms).
The primary motivation for this patch is to reduce the amount of drudgery when implementing a new algorithm. I also think that having these closely related tests consolidated together makes them more readable.
A potential downside is that we're no longer testing that the algorithms behave correctly. However, I think these checks were redundant -- the probability of an algorithm compiling but producing incorrect results when given e.g. a non-bool predicate seems to be essentially zero.
libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.compile.pass.cpp | ||
---|---|---|
40 | Originally I tried to have a single overloaded function for each kind of invocation, but I found that there is too much complexity when trying to distinguish between different invocations that have the same number of arguments (especially the unconstrained arguments like predicates, projections and values). Additionally, I think that cataloguing all the different algorithm signatures has some value. | |
109 | I avoided creating helper functions here because their main benefit is taking care of both the (iterator, sentinel) and (range) overloads which doesn't apply here. | |
126 | In general, I avoided creating a helper function unless there were at least 3 uses of it within the file. | |
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp | ||
160 | I'm reusing the name "predicate" here even though it's arguably a little hacky. |
LGTM % nits.
libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.compile.pass.cpp | ||
---|---|---|
175–179 | The main shouldn't be required anymore. Also applies to the invoke test. | |
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp | ||
2 | Please update the PR title. | |
202 | I would just omit the algorithms for which the test doesn't apply, like we do in the other robust_against_x tests. |
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp | ||
---|---|---|
202 | Hmm, I'm inclined to leave these in. It makes it easier to verify that no algorithm has been accidentally omitted. |
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp | ||
---|---|---|
202 | Do it as you see fit. I don't have any strong opinions here. I mostly wanted to note that this deviates from the other tests. |
It seems that the "ranges_robust_against_copying_xxx" tests are in "test/libcxx/algorithms" and these "ranges_robust_against_" tests are in "test/std/algorithms".
Is there a reason for using different directories?
Tests under the std directory should pass on any valid standard library implementation, while tests under libcxx are libc++-specific. It's not mandated that we don't copy comparators or projections (AFAIK, GCC does), it's just a self-imposed optimization (see https://reviews.llvm.org/D114136 for more context). OTOH, the behavior in these tests I believe is mandated by the standard, so it should go under std.
clang-format not found in user’s local PATH; not linting file.