Also simplify the robust test files for non-boolean predicates and
omitting std::invoke.
Details
- Reviewers
philnik huixie90 ldionne - Group Reviewers
Restricted Project - Commits
- rG25aa29f38a54: [libc++][ranges][NFC] Consolidate range algorithm checks for returning…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.compile.pass.cpp | ||
---|---|---|
39 | I somehow missed this in the initial implementation -- variadics make things so much simpler. |
libcxx/include/__algorithm/ranges_remove_if.h | ||
---|---|---|
23 | question. why do we need to include this header. borrowed_subrange_t is defined in subrange.h. can we consider dangling is a detail in borrowed_subrange_t? | |
libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp | ||
34 | this is not an issue for the test because the test doesn't actually run the code. but this is undefined behaviour. This makes me think that we should probably actually run the code? | |
35 | is this one used apart from called by the other constructor? | |
53 | a possible additional to the test is dangling_both | |
53 | perhaps it is outside the scope of this test, but for the existing tests in set operations, for those tests that are testing that the second argument is dangling, it also verifies the first argument is a normal iterator which points to the correct place. (but it might be over-testing) | |
155 | you can enable this test if you rebase the main. also I think this one also needs to test the 2nd case |
I think we should run the dangling tests and check their outputs. In theory we could optimize on an algorithms returning ranges::dangling. For some algorithms this would work perfectly, but for others there are side-effects where the current tests would catch the problem while this test doesn't. The nonbool and invoke changes LGTM.
Made this a runtime test and added a TODO to check the non-dangling iterators. I'm not removing any existing tests yet, so this patch is strictly improving coverage even without checking the results (and I won't remove existing checks without adding an equivalent check here). Quite a few existing tests either don't check for dangling at all or just check the return type.
I'm not convinced this check is worthwhile, by the way -- I can't think of a case where an algorithm would produce incorrect output in presence of dangling but work correctly otherwise. But if these checks could be made very brief, I can add them (or won't object if someone else does).
libcxx/include/__algorithm/ranges_remove_if.h | ||
---|---|---|
23 | I simply forgot that borrowed_subrange_t isn't defined in dangling.h. Thanks for catching this! | |
libcxx/test/std/algorithms/ranges_robust_against_dangling.compile.pass.cpp | ||
53 | Re. dangling_both -- I thought about it and it seemed a little overkill, but since you mentioned it, I decided to add it anyway. |
libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp | ||
---|---|---|
45–46 ↗ | (On Diff #445376) | I still prefer decltype(auto) result = func(R(in), std::forward<Args>(args)...); static_assert(std::same_as<decltype(result), ExpectedT>); or [[maybe_unused]] std::same_as<ExpectedT> decltype(auto) result = func(R(in), std::forward<Args>(args)...); This way the tests actually run the algorithms instead of just checking the return type. |
libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp | ||
---|---|---|
45–46 ↗ | (On Diff #445376) | Good point, done. |
LGTM w/ comments. I think @philnik's comments have been addressed.
libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.compile.pass.cpp | ||
---|---|---|
27 | I think this should be a .pass.cpp. | |
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp | ||
28 | I think this should be a .pass.cpp. |
clang-format not found in user’s local PATH; not linting file.