Page MenuHomePhabricator

[libc++][ranges][NFC] Consolidate range algorithm checks for returning `dangling`.
ClosedPublic

Authored by var-const on Jul 14 2022, 1:25 AM.

Details

Summary

Also simplify the robust test files for non-boolean predicates and
omitting std::invoke.

Diff Detail

Event Timeline

var-const created this revision.Jul 14 2022, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 1:25 AM
var-const requested review of this revision.Jul 14 2022, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 1:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const added inline comments.Jul 14 2022, 1:26 AM
libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.compile.pass.cpp
46

I somehow missed this in the initial implementation -- variadics make things so much simpler.

huixie90 added inline comments.Jul 14 2022, 2:30 AM
libcxx/include/__algorithm/ranges_remove_if.h
23 ↗(On Diff #444557)

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

this is not an issue for the test because the test doesn't actually run the code. but this is undefined behaviour.
the data_ pointer points to the temporary copy std::array arr and will be dangling immediately after the constructor returns

This makes me think that we should probably actually run the code?

35 ↗(On Diff #444557)

is this one used apart from called by the other constructor?

53 ↗(On Diff #444557)

a possible additional to the test is dangling_both

53 ↗(On Diff #444557)

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

you can enable this test if you rebase the main. also I think this one also needs to test the 2nd case

philnik requested changes to this revision.Jul 14 2022, 7:12 AM

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.

This revision now requires changes to proceed.Jul 14 2022, 7:12 AM
var-const updated this revision to Diff 445376.Jul 17 2022, 7:42 PM
var-const marked 6 inline comments as done.

Address feedback, rebase.

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

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

Re. dangling_both -- I thought about it and it seemed a little overkill, but since you mentioned it, I decided to add it anyway.

huixie90 added inline comments.Jul 18 2022, 1:43 AM
libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp
45–46

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.

var-const updated this revision to Diff 445687.Jul 18 2022, 9:03 PM

Address feedback and rebase.

var-const marked an inline comment as done.Jul 18 2022, 9:04 PM
var-const added inline comments.
libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp
45–46

Good point, done.

var-const marked an inline comment as done.Jul 19 2022, 1:30 AM

The Windows CI failure looks like a fluke.

huixie90 accepted this revision.Jul 19 2022, 10:18 AM
ldionne accepted this revision.Jul 19 2022, 1:40 PM
ldionne added a subscriber: ldionne.

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.

var-const marked 2 inline comments as done.Jul 19 2022, 5:10 PM
var-const updated this revision to Diff 445986.Jul 19 2022, 5:10 PM

Address feedback.

var-const updated this revision to Diff 445989.Jul 19 2022, 5:20 PM

Rebase on main.

var-const updated this revision to Diff 446001.Jul 19 2022, 6:20 PM

Rebase on main.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 19 2022, 8:46 PM
This revision was automatically updated to reflect the committed changes.