[libcxx-commits] [PATCH] D105456: [libcxx][algorithms] adds `std::ranges::find`
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Aug 17 14:59:42 PDT 2021
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Comments per live review. Thanks!
Would it be possible to add the algorithms to the status page?
================
Comment at: libcxx/include/CMakeLists.txt:19
__algorithm/fill.h
+ __algorithm/find.h
__algorithm/find_end.h
----------------
That entry already exists below.
================
Comment at: libcxx/include/__algorithm/find.h:66
+ requires indirect_binary_predicate<ranges::equal_to, projected<iterator_t<_Rp>, _Proj>, const _Tp*>
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr
+ borrowed_iterator_t<_Rp> operator()(_Rp&& __r, const _Tp& __value, _Proj __proj = {}) const
----------------
Can you please make sure that you test this *at runtime* with a return of `ranges::dangling`? If it's valid to call it, even though it's kinda stupid to, we should test it.
================
Comment at: libcxx/include/__algorithm/find.h:67
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr
+ borrowed_iterator_t<_Rp> operator()(_Rp&& __r, const _Tp& __value, _Proj __proj = {}) const
+ {
----------------
`__r` -> `__range`?
================
Comment at: libcxx/test/libcxx/diagnostics/nodiscard_ranges_extensions.verify.cpp:12
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
+
----------------
Not needed anymore.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/find_iter_sent_proj.pass.cpp:15
+
+// ranges::find(r, value, proj)
+
----------------
For all the overloads that accept a projection, I would really like it if we could test at least one usage of a pointer to member, since that is basically the first (and often the only) use they will get by users. It would be a shame if that didn't work, so I'd like to see that exercised.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/find_iter_sent_proj.pass.cpp:44
+ auto input = make_archetype_range<I>(r);
+ std::same_as<iterator> auto const result = ranges::find(ranges::begin(input), ranges::end(input), value, proj);
+ assert(*result == expected_value);
----------------
Assuming you parameterize the other `ranges::find` on the range instead of the iterator, we should even be able to replace this by `ranges::find(I(ranges::begin(input)), I(ranges::end(input)), ...)` and get rid of `make_archetype_range` altogether.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/find_iter_sent_proj.pass.cpp:58
+template<template<class...> class I, ranges::input_range R, class T, class Proj>
+constexpr void check_find_fail(R&& r, T const value, Proj proj)
+{
----------------
You could pass the expected complexity here and avoid calling `ranges::ssize`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find_range.pass.cpp:43
+ auto input = make_archetype_range<I>(r);
+ std::same_as<iterator> auto const result = ranges::find(input, value);
+ assert(*result == value);
----------------
In this test, we end up only testing `ranges::find` on a `ranges::subrange<whatever>`, because that is what `make_archetype_range` returns. We should also test it on other range archetypes to make sure it works as expected.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find_range.pass.cpp:66-92
+template<template<class...> class I, ranges::input_range R, class T>
+constexpr void check_find_fail(R&& r, T const value)
+{
+ using iterator = I<ranges::iterator_t<R>>;
+ {
+ {
+ auto input = make_archetype_range<I>(r);
----------------
For this overload of `ranges::find` which takes a range as an input, I think it would make sense to instead parameterize the various tests on the range type instead of the iterator type. That would also solve my comment above regarding this test only checking variants of `ranges::subrange`. I think something like my suggestion would work.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find_range.pass.cpp:97
+{
+ constexpr auto range = std::array{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+ check_find_success<I>(range, 0, range.begin() + 0, 1);
----------------
I like that we're testing various positions within the range, but we're always testing the same range. It would be useful to also test an empty range and a one-element range at least, WDYT?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find_range.pass.cpp:99
+ check_find_success<I>(range, 0, range.begin() + 0, 1);
+ check_find_success<I>(range, 1, range.begin() + 1, 2);
+ check_find_success<I>(range, 2, range.begin() + 2, 3);
----------------
Suggestion: Consider using an array that contains something like characters (`'0', '1', ...) to avoid a correspondence between the value in the range and its position.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/ranges_find_range.pass.cpp:142-143
+{
+ static_assert(check_find<cpp17_input_iterator>());
+ check_find<cpp17_input_iterator>();
+
----------------
Elsewhere in the test suite, we start with the runtime version and then do the `static_assert`. It's really a nitpick, but it would be good to be consistent since there's otherwise no difference between the two.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/requirements.compile.pass.cpp:13
+
+// ranges::find
+
----------------
Can you please add a comment explaining what this test does at a high level?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/requirements.compile.pass.cpp:25-77
+template<class T>
+constexpr bool check_iterator_requirements()
+{
+ using R = output_range<T>;
+ static_assert(!requires(R r) { ranges::find(r.begin(), r.end(), T()); });
+ static_assert(!requires(R r) { ranges::find(r.begin(), r.end(), 0, &weakly_incrementable_pair::first); });
+ return true;
----------------
I suggest you rewrite this into a single function. That will simplify the test and avoid mistakes such as forgetting to call one of these functions (it seems like `check_iterator_requirements` is not being called at all right now).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges_find/special_function.compile.pass.cpp:78
+void find(iterator<I>, iterator<I>, std::pair<int, int> const&) {
+ static_assert(std::same_as<I, I*>);
+}
----------------
I would suggest this instead:
```
template <class I, bool AlwaysFalse = false>
void find(...) {
static_assert(AlwaysFalse);
}
```
Alternatively, we could have a helper like `dependent_false<...>` or `always_false<T...>` and use that.
================
Comment at: libcxx/test/support/test_algorithm.h:22-29
+// `complexity_invocable` is an invocable adaptor that's used to measure the number of calls to a
+// predicate or projection. It differs from `unary_counting_predicate` and `binary_counting_predicate`
+// in two ways:
+// 1. Most importantly, it isn't limited to predicates, and so works with invocables returning
+// something other than Boolean values.
+// 2. It can store lambdas and works in constexpr contexts.
+template<class F>
----------------
I believe `counting_invocable` would convey what this class does better. I've been a bit confused by the use of `complexity` ever since I saw this patch. Also, we could move it to `counting_predicates.h` so it's alongside other similar helpers (and consider renaming that header if you want).
Also, I don't think the (1) and (2) points in the comment are necessary - those are like a justification for adding this type instead of reusing another one, but IMO we don't need that justification.
================
Comment at: libcxx/test/support/test_iterators.h:1002-1003
+// `complexity_iterator` is an iterator adaptor that's used to measure the number of applications of
+// a predicate/projection when there's no predicate to measure (e.g. `ranges::find`).
+template<std::input_iterator I>
----------------
I believe this should instead say that the intention is to measure the number of applications of the predicate, but that this iterator actually achieves that by counting the number of dereferences.
================
Comment at: libcxx/test/support/test_iterators.h:1005
+template<std::input_iterator I>
+class complexity_iterator {
+public:
----------------
As for `complexity_invocable`, I'm not a big fan of `complexity` here. How about `counting_iterator`?
================
Comment at: libcxx/test/support/test_range.h:66
+template<class T>
+class input_range {
+public:
----------------
What about using `test_input_range`?
================
Comment at: libcxx/test/support/test_range.h:75
+template<class T>
+class output_range {
+public:
----------------
`test_output_range`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105456/new/
https://reviews.llvm.org/D105456
More information about the libcxx-commits
mailing list