[PATCH] D136522: [libcxx] patch for implementing ranges::find_last
Konstantin Varlamov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 2 19:12:49 PDT 2023
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
So sorry it took me so long to get to this review!
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:15
+#include <__functional/identity.h>
+#include <__functional/invoke.h>
+#include <__functional/ranges_operations.h>
----------------
Is this used?
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:16
+#include <__functional/invoke.h>
+#include <__functional/ranges_operations.h>
+#include <__iterator/concepts.h>
----------------
Is this used?
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:37-38
+ template <forward_iterator _Ip, sentinel_for<_Ip> _Sp, class _Tp, class _Proj = identity>
+ requires indirect_binary_predicate < ranges::equal_to, projected<_Ip, _Proj>,
+ const _Tp* > _LIBCPP_HIDE_FROM_ABI constexpr subrange<_Ip>
+ operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
----------------
This formatting looks really weird, is it produced by `clang-format`? (If so, feel free to disable it for these lines by adding a `// clang-format off` comment, then reenable with `// clang-format on`)
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:38
+ requires indirect_binary_predicate < ranges::equal_to, projected<_Ip, _Proj>,
+ const _Tp* > _LIBCPP_HIDE_FROM_ABI constexpr subrange<_Ip>
+ operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
----------------
Can you also add `_LIBCPP_NODISCARD_EXT` to all the new algorithms? We want to mark these as `[[nodiscard]]` since they have no side effects and are only used to produce a value, but since they aren't marked as such in the Standard, we have to do it as an extension. Please also update the `test/libcxx/diagnostics/ranges.nodiscard_extensions.verify.cpp` test file.
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:40
+ operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
+ auto __pred = [&](auto&& __e) { return std::forward<decltype(__e)>(__e) == __value; };
+ return ranges::__find_last_if_impl(std::move(__first), std::move(__last), __pred, __proj);
----------------
Question: what's the case where `forward` would make a difference? Do we have a test for that?
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:61
+
+#endif // _LIBCPP_STD_VER > 23
+
----------------
`>= 23`.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:38
+__find_last_if_impl(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj) {
+ _Ip __current = __first;
+ subrange<_Ip> __result{__last, __last};
----------------
Normally we simply increment `__first`, no need to create a separate variable.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:38-46
+ _Ip __current = __first;
+ subrange<_Ip> __result{__last, __last};
+ while (__current != __last) {
+ if (std::invoke(__pred, std::invoke(__proj, *__current))) {
+ __result = {__current, __last};
+ }
+ ++__current;
----------------
var-const wrote:
> Normally we simply increment `__first`, no need to create a separate variable.
Recreating the whole `__result` each time seems a little wasteful, consider:
```
_Ip __result = __last;
while (__first != __last) {
if (std::invoke(__pred, std::invoke(__proj, * __first))) {
__result = __first;
}
++__first;
}
return {__result, __last};
```
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:44
+ }
+ ++__current;
+ }
----------------
For a bidirectional common range, it seems more efficient to iterate from the end -- then you can return as soon as you find an element satisfying the predicate. For a bidirectional non-common range, it might still be more efficient to create an iterator from the sentinel and go from the end (though this is debatable).
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:54
+ indirect_unary_predicate<projected<iterator_t<_Rp>, _Proj>> _Pred>
+ _LIBCPP_NODISCARD_EXT constexpr borrowed_subrange_t<_Rp>
+ operator()(_Rp&& __r, _Pred __pred, _Proj __proj = {}) const {
----------------
Missing `_LIBCPP_HIDE_FROM_ABI`.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:57
+ return ranges::__find_last_if_impl(ranges::begin(__r), ranges::end(__r), __pred, __proj);
+ }
+ template <forward_iterator _Ip,
----------------
Please add a newline after this line.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:62
+ indirect_unary_predicate<projected<_Ip, _Proj>> _Pred>
+ _LIBCPP_NODISCARD_EXT constexpr subrange<_Ip>
+ operator()(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj = {}) const {
----------------
Missing `_LIBCPP_HIDE_FROM_ABI`.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:76
+
+#endif // _LIBCPP_STD_VER > 23
+
----------------
`>= 23`.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if_not.h:15
+#include <__functional/identity.h>
+#include <__functional/invoke.h>
+#include <__functional/ranges_operations.h>
----------------
`iterator_operations.h` might be unnecessary.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if_not.h:42
+ operator()(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj = {}) const {
+ auto __not_pred = [&](auto&& __e) { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
+ return ranges::__find_last_if_impl(std::move(__first), std::move(__last), __not_pred, __proj);
----------------
Do we have a test that fails if we remove this `forward`?
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if_not.h:51
+ operator()(_Rp&& __r, _Pred __pred, _Proj __proj = {}) const {
+ auto __not_pred = [&](auto&& __e) { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
+ return ranges::__find_last_if_impl(ranges::begin(__r), ranges::end(__r), __not_pred, __proj);
----------------
To avoid duplicating this logic, consider instead delegating to the iterator version of `find_last_if_not`.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if_not.h:64
+
+#endif // _LIBCPP_STD_VER > 23
+
----------------
`>= 23`
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if_not.h:42
+ operator()(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj = {}) const {
+ auto __pred2 = [&](auto&& __e) { return !std::invoke(__pred, std::forward<decltype(__e)>(__e)); };
+ return ranges::__find_last_if_impl(std::move(__first), std::move(__last), __pred2, __proj);
----------------
Mordante wrote:
> naming is hard, but let's try to use something better than just a number.
> How about `__not_pred` since this is basically the inverse of the predicate given.
>
> Another option is just not giving this predicate a name and write it in the return statement on the next line.
Very optional: `__negate_pred` or `__negated_pred`?
================
Comment at: libcxx/include/algorithm:102
+ template<forward_iterator I, sentinel_for<I> S, class T, class Proj = identity>
+ requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
----------------
Nit: please fix indentation here.
================
Comment at: libcxx/include/algorithm:104
+ requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
+ constexpr subrange<I> ranges::find_last(I first, S last, const T& value, Proj proj = {}); // since C++23
+
----------------
Nit: can these comments (`// since C++23`) be aligned?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp:145
+ {
+ // check that std::invoke is used
+ struct S {
----------------
This test should also apply to the iterator version of the algorithm, right? (applies to the other two test files as well)
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp:164
+ {
+ int a[] = {1, 2, 2, 3, 4};
+ int projection_count = 0;
----------------
Wrong indentation.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp:175
+ {
+ OneWayComparable a[] = {OneWayComparable{true}};
+ auto ret = std::ranges::find_last(a, a + 1, OneWayComparable{false});
----------------
Do the other two files need an equivalent test?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp:215
+ std::array<int, 0> a = {};
+ constexpr int search_value = 1;
+ auto ret = std::ranges::find_last(a.begin(), a.end(), search_value);
----------------
Does this have to be `constexpr`?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp:230
+ {
+ StrictComparable<int> a[] = {1, 2, 2, 3, 4};
+ auto ret = std::ranges::find_last(a, a + 4, StrictComparable<int>{2});
----------------
The other files use `BooleanComparable`, can this be made consistent?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp:81
+ int a[] = {1, 2, 3, 4};
+ auto ret = std::ranges::find_last(a, a + 4, a + 3, [](int& i) { return &i; });
+ assert(ret.data() == a + 3);
----------------
Mordante wrote:
> Here you should use `std::same_as<return type> auto` method. For example have a look at
> `libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp`
Looks not done?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:9
+
+// <algorithm>
+
----------------
Note: most comments in this file apply to the other two test files as well.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:35
+template <class It, class Sent = It>
+concept HasFindIfIt = requires(It it, Sent sent) {
+ std::ranges::find_last_if(it, sent, Predicate{});
----------------
`s/HasFindIfIt/HasFindLastIfIt/`, same for the range version below.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:53
+
+static_assert(!HasFindIfPred<IndirectUnaryPredicateNotCopyConstructible>);
+static_assert(!HasFindIfPred<IndirectUnaryPredicateNotPredicate>);
----------------
Please also add a case where `HasFindLastIfPred` is `true` (to test the test, so to speak).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:71
+ {
+ int a[] = {1, 2, 3, 4};
+ std::same_as<std::ranges::subrange<It>> auto ret =
----------------
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.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:73
+ 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; });
+ assert(base(ret.begin()) == a + 3);
----------------
This formatting is incorrect, continuations should be indented.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:73
+ 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; });
+ assert(base(ret.begin()) == a + 3);
----------------
var-const wrote:
> This formatting is incorrect, continuations should be indented.
Why is it `mutable`?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:78
+ {
+ int a[] = {1, 2, 3, 4};
+ auto range = std::ranges::subrange(It(a), Sent(It(a + 4)));
----------------
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.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:87
+
+struct NonConstComparable {
+ friend constexpr bool operator==(const NonConstComparable&, const NonConstComparable&) { return false; }
----------------
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?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:88
+struct NonConstComparable {
+ friend constexpr bool operator==(const NonConstComparable&, const NonConstComparable&) { return false; }
+ friend constexpr bool operator==(NonConstComparable&, NonConstComparable&) { return false; }
----------------
Are all 4 overloads necessary? If so, please add a short comment to explain why.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:95
+constexpr bool test() {
+ test_iterators<int*>();
+ test_iterators<const int*>();
----------------
Consider using `types::for_each` to cut down on the boilerplate a little.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:97
+ test_iterators<const int*>();
+ test_iterators<bidirectional_iterator<int*>>();
+ test_iterators<forward_iterator<int*>>();
----------------
Can these types be ordered from most to least restrictive? (or vice versa)
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:102
+
+ { // check that projections are used properly and that they are called with the iterator directly
+ {
----------------
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).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:106
+ auto ret = std::ranges::find_last_if(
+ a, a + 4, [&](int* i) { return i == a + 3; }, [](int& i) { return &i; });
+ assert(ret.data() == a + 3);
----------------
Please make the projection a named variable and reuse it between the iterator version and the range version.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:107-108
+ a, a + 4, [&](int* i) { return i == a + 3; }, [](int& i) { return &i; });
+ assert(ret.data() == a + 3);
+ }
+ {
----------------
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.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:121
+ int comp;
+ int other;
+ };
----------------
How about `s/other/index/`?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:130
+ {
+ struct S {
+ int comp;
----------------
(throughout) You can reuse these helper types between the iterator version and the range version to reduce boilerplate.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:143
+
+ { // check that end + 1 iterator is returned with no match
+ {
----------------
Nit: `s/end + 1/past-the-end/`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:143
+
+ { // check that end + 1 iterator is returned with no match
+ {
----------------
var-const wrote:
> Nit: `s/end + 1/past-the-end/`.
I would move this test to `test_iterators`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:158
+ // check that ranges::dangling is returned
+ [[maybe_unused]] std::same_as<std::ranges::dangling> auto ret =
+ std::ranges::find_last_if(std::array{1, 2}, [](int) { return false; });
----------------
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.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:182
+ int a[] = {1, 2, 2, 3, 4};
+ int predicate_count = 0;
+ int projection_count = 0;
----------------
Please see if the types from `test/support/counting_{projection,predicates}.h` can be used here.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:208
+
+ { // check that the return type of `iter::operator*` doesn't change
+ {
----------------
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).
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:221
+
+ { // check that an empty range works
+ {
----------------
Please move this test case to `test_iterators`.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if_not.pass.cpp:73
+ std::same_as<std::ranges::subrange<It>> auto ret =
+ std::ranges::find_last_if_not(It(a), Sent(It(a + 5)), [c = 0](int) mutable { return c++ <= 2; });
+ assert(base(ret.begin()) == a + 4);
----------------
Can we use a simpler predicate? These tests should check the most basic logic and be as easy to read as possible.
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if_not.pass.cpp:118
+ {
+ // check that the first element is returned
+ {
----------------
Last element?
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if_not.pass.cpp:254
+
+ return 0;
+}
----------------
Please also update `test/std/library/description/conventions/customization.point.object/niebloid.compile.pass.cpp` and the `*robust*` test files where applicable (searching for `ranges::find_if` among the `*robust*` test files should give a good idea of which ones to update).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136522/new/
https://reviews.llvm.org/D136522
More information about the llvm-commits
mailing list