This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges][NFC] Consolidate some repetitive range algorithm tests:
ClosedPublic

Authored by var-const on Jul 8 2022, 6:10 PM.

Details

Summary
  • 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.

Diff Detail

Event Timeline

var-const created this revision.Jul 8 2022, 6:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 6:10 PM
var-const updated this revision to Diff 443404.Jul 8 2022, 6:26 PM

Add comment.

philnik added a subscriber: philnik.Jul 8 2022, 6:36 PM

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

Nit: Add a newline between <> and "" includes

77

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

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.

var-const updated this revision to Diff 443544.Jul 10 2022, 8:04 PM
var-const marked 3 inline comments as done.

Add tests for omitting std::invoke.

var-const published this revision for review.Jul 10 2022, 8:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2022, 8:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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
39 ↗(On Diff #443544)

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.

108 ↗(On Diff #443544)

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.

125 ↗(On Diff #443544)

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
159 ↗(On Diff #443544)

I'm reusing the name "predicate" here even though it's arguably a little hacky.

var-const updated this revision to Diff 443743.Jul 11 2022, 1:36 PM

Fix the CI.

philnik accepted this revision.Jul 11 2022, 1:40 PM

LGTM % nits.

libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.compile.pass.cpp
174–178 ↗(On Diff #443743)

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
1 ↗(On Diff #443743)

Please update the PR title.

201 ↗(On Diff #443743)

I would just omit the algorithms for which the test doesn't apply, like we do in the other robust_against_x tests.

This revision is now accepted and ready to land.Jul 11 2022, 1:40 PM
var-const retitled this revision from [libc++][ranges][NFC] Consolidate range algorithm tests with non-boolean predicates. to [libc++][ranges][NFC] Consolidate some repetitive range algorithm tests:.Jul 11 2022, 1:43 PM
var-const edited the summary of this revision. (Show Details)
var-const marked 3 inline comments as done.
var-const added inline comments.
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp
201 ↗(On Diff #443743)

Hmm, I'm inclined to leave these in. It makes it easier to verify that no algorithm has been accidentally omitted.

var-const updated this revision to Diff 443747.Jul 11 2022, 1:45 PM
var-const marked an inline comment as done.

Address feedback

philnik added inline comments.Jul 11 2022, 1:48 PM
libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.compile.pass.cpp
201 ↗(On Diff #443743)

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?

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.