This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove tests from ranges.pass.cpp which violate semantic requirements

Authored by philnik on May 15 2023, 9:45 AM.



This also removes some tests which we have grouped together into robust_from_*.pass.cpp tests.

Specifically, checking that

  • ranges::dangling is returned is done in libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp
  • std::invoke is used is done in libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.pass.cpp.
  • implicit conversion to bool works is done in libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.pass.cpp

Checking the comparison order is invalid because the operator== isn't symmetric.
Checking what the exact type of operator== is, is invalid because comparing the same object has to yield the same results if the objects are not modified.

Diff Detail

Event Timeline

philnik created this revision.May 15 2023, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 9:45 AM
philnik requested review of this revision.May 15 2023, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 9:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.May 16 2023, 12:15 PM
EricWF added a subscriber: EricWF.

What semantic requirements do these violate. Please add that information to the description.

This revision now requires changes to proceed.May 16 2023, 12:15 PM
ldionne accepted this revision as: ldionne.May 17 2023, 8:36 AM

@philnik Please document why those tests are violating constraints or redundant in your commit message, but otherwise this LGTM with the above justifications.

@EricWF Please let us know if you still see a problem with this patch after these explanations.


This one is already tested in libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp.


This one is libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.pass.cpp.


This tried to check that we are calling *it == value exactly in the algorithm. However, equality_comparable_with and in particular __WeaklyEqualityComparableWith ( require that both t == u and u == t be valid, which means that this is not technically required to work.


My understanding is that this test was trying to check that we don't artificially add a const qualifier to the result of *it when we do *it == value. However, per

template<class T, class U>
concept __WeaklyEqualityComparableWith = // exposition only
  requires(const std::remove_reference_t<T>& t,
           const std::remove_reference_t<U>& u) {
    { t == u } -> boolean-testable;
    { t != u } -> boolean-testable;
    { u == t } -> boolean-testable;
    { u != t } -> boolean-testable;

This always adds a const qualifier to t and u, so we're allowed to rely on that.


This is libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.pass.cpp.

philnik edited the summary of this revision. (Show Details)May 17 2023, 9:51 AM
ldionne accepted this revision.May 22 2023, 8:54 AM

@EricWF Gentle ping -- does this look good to you with the added justifications? @philnik, let's land this tomorrow unless @EricWF raises additional concerns, otherwise we can address them in a follow-up.

This revision was not accepted when it landed; it landed in state Needs Revision.May 23 2023, 8:59 AM
This revision was automatically updated to reflect the committed changes.