Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D136522 Diff 516117 libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp

Changeset View

Standalone View

View Options

libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp

  • This file was added.
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// <algorithm>
var-constUnsubmitted
Not Done
ReplyInline Actions

Note: most comments in this file apply to the other two test files as well.

var-const: Note: most comments in this file apply to the other two test files as well.
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// template<forward_iterator I, sentinel_for<I> S, class Proj = identity,
// indirect_unary_predicate<projected<I, Proj>> Pred>
// constexpr subrange<I> ranges::find_last_if(I first, S last, Pred pred, Proj proj = {});
// template<forward_range R, class Proj = identity,
// indirect_unary_predicate<projected<iterator_t<R>, Proj>> Pred>
// constexpr borrowed_subrange_t<R>
// ranges::find_last_if(R&& r, Pred pred, Proj proj = {});
#include <algorithm>
#include <array>
#include <cassert>
#include <ranges>
#include "almost_satisfies_types.h"
#include "boolean_testable.h"
#include "test_iterators.h"
struct Predicate {
bool operator()(int);
};
template <class It, class Sent = It>
concept HasFindIfIt = requires(It it, Sent sent) {
var-constUnsubmitted
Not Done
ReplyInline Actions

s/HasFindIfIt/HasFindLastIfIt/, same for the range version below.

var-const: `s/HasFindIfIt/HasFindLastIfIt/`, same for the range version below.
std::ranges::find_last_if(it, sent, Predicate{});
};
static_assert(HasFindIfIt<int*>);
static_assert(!HasFindIfIt<InputIteratorNotDerivedFrom>);
static_assert(!HasFindIfIt<InputIteratorNotIndirectlyReadable>);
static_assert(!HasFindIfIt<InputIteratorNotInputOrOutputIterator>);
static_assert(!HasFindIfIt<cpp20_input_iterator<int*>, SentinelForNotSemiregular>);
static_assert(!HasFindIfIt<cpp20_input_iterator<int*>, InputRangeNotSentinelEqualityComparableWith>);
static_assert(!HasFindIfIt<int*, int>);
static_assert(!HasFindIfIt<int, int*>);
template <class Pred>
concept HasFindIfPred = requires(int* it, Pred pred) {
std::ranges::find_last_if(it, it, pred);
};
static_assert(!HasFindIfPred<IndirectUnaryPredicateNotCopyConstructible>);
var-constUnsubmitted
Not Done
ReplyInline Actions

Please also add a case where HasFindLastIfPred is true (to test the test, so to speak).

var-const: Please also add a case where `HasFindLastIfPred` is `true` (to test the test, so to speak).
static_assert(!HasFindIfPred<IndirectUnaryPredicateNotPredicate>);
template <class R>
concept HasFindIfR = requires(R r) {
std::ranges::find_last_if(r, Predicate{});
};
static_assert(HasFindIfR<std::array<int, 0>>);
static_assert(!HasFindIfR<int>);
static_assert(!HasFindIfR<InputRangeNotDerivedFrom>);
static_assert(!HasFindIfR<InputRangeNotIndirectlyReadable>);
static_assert(!HasFindIfR<InputRangeNotInputOrOutputIterator>);
static_assert(!HasFindIfR<InputRangeNotSentinelSemiregular>);
static_assert(!HasFindIfR<InputRangeNotSentinelEqualityComparableWith>);
template <class It, class Sent = It>
constexpr void test_iterators() {
{
int a[] = {1, 2, 3, 4};
var-constUnsubmitted
Not Done
ReplyInline Actions

Can we add a few more test cases?

  • empty range;
  • 1-element range;
  • no element satisfies the predicate;
  • all elements satisfy the predicate;
  • the element being searched is the first one;
  • the element being searched is the last one.
var-const: Can we add a few more test cases? - empty range; - 1-element range; - no element satisfies the…
std::same_as<std::ranges::subrange<It>> auto ret =
std::ranges::find_last_if(It(a), Sent(It(a + 4)), [](int x) mutable { return x == 4; });
var-constUnsubmitted
Not Done
ReplyInline Actions

This formatting is incorrect, continuations should be indented.

var-const: This formatting is incorrect, continuations should be indented.
var-constUnsubmitted
Not Done
ReplyInline Actions

Why is it mutable?

var-const: Why is it `mutable`?
assert(base(ret.begin()) == a + 3);
assert(*ret.begin() == 4);
}
{
int a[] = {1, 2, 3, 4};
var-constUnsubmitted
Not Done
ReplyInline Actions

Please reuse the input between the iterator version and the range version, it cuts down boilerplate and makes it easier to verify that the two test cases are equivalent.

var-const: Please reuse the input between the iterator version and the range version, it cuts down…
auto range = std::ranges::subrange(It(a), Sent(It(a + 4)));
std::same_as<std::ranges::subrange<It>> auto ret =
std::ranges::find_last_if(range, [](int x) mutable { return x == 4; });
assert(base(ret.begin()) == a + 3);
assert(*ret.begin() == 4);
}
}
struct NonConstComparable {
var-constUnsubmitted
Not Done
ReplyInline Actions

It's better if the tests for these very similar algorithms are as uniform as possible. The find_last_if_not test file calls this helper NonConstComparableLvalue while the find_last test file has both NonConstComparableLValue and NonConstComparableRValue . Can this be consistent?

var-const: It's better if the tests for these very similar algorithms are as uniform as possible. The…
friend constexpr bool operator==(const NonConstComparable&, const NonConstComparable&) { return false; }
var-constUnsubmitted
Not Done
ReplyInline Actions

Are all 4 overloads necessary? If so, please add a short comment to explain why.

var-const: Are all 4 overloads necessary? If so, please add a short comment to explain why.
friend constexpr bool operator==(NonConstComparable&, NonConstComparable&) { return false; }
friend constexpr bool operator==(const NonConstComparable&, NonConstComparable&) { return false; }
friend constexpr bool operator==(NonConstComparable&, const NonConstComparable&) { return true; }
};
constexpr bool test() {
test_iterators<int*>();
var-constUnsubmitted
Not Done
ReplyInline Actions

Consider using types::for_each to cut down on the boilerplate a little.

var-const: Consider using `types::for_each` to cut down on the boilerplate a little.
test_iterators<const int*>();
test_iterators<bidirectional_iterator<int*>>();
var-constUnsubmitted
Not Done
ReplyInline Actions

Can these types be ordered from most to least restrictive? (or vice versa)

var-const: Can these types be ordered from most to least restrictive? (or vice versa)
test_iterators<forward_iterator<int*>>();
test_iterators<random_access_iterator<int*>>();
test_iterators<contiguous_iterator<int*>>();
{ // check that projections are used properly and that they are called with the iterator directly
var-constUnsubmitted
Not Done
ReplyInline Actions

Can you rephrase the called with the iterator directly bit? It reads as if the projection receives an iterator as an argument, which isn't the case. I think something like called with the reference to the element the iterator is pointing to would be more technically correct (though this can definitely be improved).

var-const: Can you rephrase the `called with the iterator directly` bit? It reads as if the projection…
{
int a[] = {1, 2, 3, 4};
auto ret = std::ranges::find_last_if(
a, a + 4, [&](int* i) { return i == a + 3; }, [](int& i) { return &i; });
MordanteUnsubmitted
Not Done
ReplyInline Actions

Please use clang-format on these tests, this looks wrong.

Mordante: Please use clang-format on these tests, this looks wrong.
var-constUnsubmitted
Not Done
ReplyInline Actions

Please make the projection a named variable and reuse it between the iterator version and the range version.

var-const: Please make the projection a named variable and reuse it between the iterator version and the…
assert(ret.data() == a + 3);
}
var-constUnsubmitted
Not Done
ReplyInline Actions

Optional: consider adding blank lines between scopes (throughout). I find code easier to read when it's split into logical blocks, and different scopes offer a very straightforward point for splitting.

var-const: Optional: consider adding blank lines between scopes (throughout). I find code easier to read…
{
int a[] = {1, 2, 3, 4};
auto ret = std::ranges::find_last_if(
a, [&](int* i) { return i == a + 3; }, [](int& i) { return &i; });
assert(ret.data() == a + 3);
}
}
{ // check that the last element is returned
{
struct S {
int comp;
int other;
var-constUnsubmitted
Not Done
ReplyInline Actions

How about s/other/index/?

var-const: How about `s/other/index/`?
};
S a[] = {{0, 0}, {0, 2}, {0, 1}};
auto ret = std::ranges::find_last_if(a, [](int i) { return i == 0; }, &S::comp);
assert(ret.data() == a + 2);
assert(ret.data()->comp == 0);
assert(ret.data()->other == 1);
}
{
struct S {
var-constUnsubmitted
Not Done
ReplyInline Actions

(throughout) You can reuse these helper types between the iterator version and the range version to reduce boilerplate.

var-const: (throughout) You can reuse these helper types between the iterator version and the range…
int comp;
int other;
};
S a[] = {{0, 0}, {0, 2}, {0, 1}};
auto ret = std::ranges::find_last_if(
a, a + 3, [](int i) { return i == 0; }, &S::comp);
assert(ret.data() == a + 2);
assert(ret.data()->comp == 0);
assert(ret.data()->other == 1);
}
}
{ // check that end + 1 iterator is returned with no match
var-constUnsubmitted
Not Done
ReplyInline Actions

Nit: s/end + 1/past-the-end/.

var-const: Nit: `s/end + 1/past-the-end/`.
var-constUnsubmitted
Not Done
ReplyInline Actions

I would move this test to test_iterators.

var-const: I would move this test to `test_iterators`.
{
int a[] = {1, 1, 1};
auto ret = std::ranges::find_last_if(a, a + 3, [](int) { return false; });
assert(ret.data() == a + 3);
}
{
int a[] = {1, 1, 1};
auto ret = std::ranges::find_last_if(a, [](int) { return false; });
assert(ret.data() == a + 3);
}
}
{
// check that ranges::dangling is returned
[[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
var-constUnsubmitted
Not Done
ReplyInline Actions

Nit: I'd use decltype(auto). In this case the difference isn't really relevant, but IMO it's better to just always use decltype(auto) which catches the exact return type and never have to think about it.

var-const: Nit: I'd use `decltype(auto)`. In this case the difference isn't really relevant, but IMO it's…
std::ranges::find_last_if(std::array{1, 2}, [](int) { return false; });
}
{
// check that an iterator is returned with a borrowing range
int a[] = {1, 1, 2, 3, 4};
auto ret = std::ranges::find_last_if(std::views::all(a), [](int) { return true; });
assert(ret.data() == a + 4);
}
{
// check that std::invoke is used
struct S {
int i;
};
S a[] = {S{1}, S{3}, S{2}};
auto ret = std::ranges::find_last_if(a, [](int) { return false; }, &S::i);
assert(ret.data() == a + 3);
}
{ // count projection and predicate invocation count
{
int a[] = {1, 2, 2, 3, 4};
int predicate_count = 0;
var-constUnsubmitted
Not Done
ReplyInline Actions

Please see if the types from test/support/counting_{projection,predicates}.h can be used here.

var-const: Please see if the types from `test/support/counting_{projection,predicates}.h` can be used here.
int projection_count = 0;
auto ret = std::ranges::find_last_if(a, a + 5, [&](int i) { ++predicate_count; return i == 2; }, [&](int i) {
++projection_count;
return i;
});
assert(ret.data() == a + 2);
assert(*(ret.data()) == 2);
assert(predicate_count == 5);
assert(projection_count == 5);
}
{
int a[] = {1, 2, 2, 3, 4};
int predicate_count = 0;
int projection_count = 0;
auto ret = std::ranges::find_last_if(a, [&](int i) { ++predicate_count; return i == 2; }, [&](int i) {
++projection_count;
return i;
});
assert(ret.data() == a + 2);
assert(*(ret.data()) == 2);
assert(predicate_count == 5);
assert(projection_count == 5);
}
}
{ // check that the return type of `iter::operator*` doesn't change
var-constUnsubmitted
Not Done
ReplyInline Actions

Can you please elaborate a little on this comment? I'm not sure how it relates to the test (which seems to check whether a predicate that takes arguments by a non-const reference works with the algorithm).

var-const: Can you please elaborate a little on this comment? I'm not sure how it relates to the test…
{
NonConstComparable a[] = {NonConstComparable{}};
auto ret = std::ranges::find_last_if(a, a + 1, [](auto&& e) { return e == NonConstComparable{}; });
assert(ret.data() == a);
}
{
NonConstComparable a[] = {NonConstComparable{}};
auto ret = std::ranges::find_last_if(a, [](auto&& e) { return e == NonConstComparable{}; });
assert(ret.data() == a);
}
}
{ // check that an empty range works
var-constUnsubmitted
Not Done
ReplyInline Actions

Please move this test case to test_iterators.

var-const: Please move this test case to `test_iterators`.
{
std::array<int, 0> a = {};
auto ret = std::ranges::find_last_if(a.begin(), a.end(), [](int) { return true; });
assert(ret.data() == a.begin());
}
{
std::array<int, 0> a = {};
auto ret = std::ranges::find_last_if(a, [](int) { return true; });
assert(ret.data() == a.begin());
}
fsb4000Unsubmitted
Done
ReplyInline Actions

static_assert

fsb4000: `static_assert`
}
{
// check that the implicit conversion to bool works
{
int a[] = {1, 2, 3, 3, 4};
auto ret = std::ranges::find_last_if(a, a + 4, [](const int& i) { return BooleanTestable{i == 3}; });
assert(ret.data() == a + 3);
}
{
int a[] = {1, 2, 3, 3, 4};
auto ret = std::ranges::find_last_if(a, [](const int& b) { return BooleanTestable{b == 3}; });
assert(ret.data() == a + 3);
}
}
return true;
}
int main(int, char**) {
test();
static_assert(test());
return 0;
}
No newline at end of file