[PATCH] D136522: [libcxx] patch for implementing ranges::find_last
Mark de Wever via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 22 10:13:29 PDT 2023
Mordante added a comment.
Thanks for your patch. I only want over it lightly. I will do an full review after the current open issues are addressed.
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:55
+inline namespace __cpo {
+inline constexpr auto find_last = __find_last::__fn{};
+} // namespace __cpo
----------------
Please run clang-format on new files, I'm quite sure this indention is off.
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:29
+
+#if _LIBCPP_STD_VER > 17
+
----------------
huixie90 wrote:
> is the paper C++23? If so, should it be
> ```
> #if _LIBCPP_STD_VER >= 23
> ```
> same applies to other files in the patch
@phyBrackets can you mark items as done when they are done? That makes reviewing these changes a lot easier.
================
Comment at: libcxx/include/__algorithm/ranges_find_last.h:39
+ _LIBCPP_HIDE_FROM_ABI constexpr
+ subrange<_Ip> operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
+ auto __pred = [&](auto&& __e) { return std::forward<decltype(__e)>(__e) == __value; };
----------------
huixie90 wrote:
> question. can we use `static operator()` now?
Why would that not be possible, I've seen this used in other ranges patches too.
================
Comment at: libcxx/include/__algorithm/ranges_find_last_if.h:38
+__find_last_if_impl(_Ip __first, _Sp __last, _Pred __pred, _Proj __proj) {
+ auto current = __first;
+ subrange<_Ip> result{__last, __last};
----------------
Please don't use `auto` everywhere, this just makes things harder to read.
Please make sure all names are __uglified, for example result too,
================
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);
----------------
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.
================
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*>
----------------
This indention is off.
================
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
----------------
Please align all these `since` entries.
================
Comment at: libcxx/include/module.modulemap.in:365
module ranges_find_if_not { private header "__algorithm/ranges_find_if_not.h" }
+ module ranges_find_last { private header "__algorithm/ranges_find_last.h" }
+ module ranges_find_last_if { private header "__algorithm/ranges_find_last_if.h" }
----------------
Please fix the alignment of the `{`.
================
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);
----------------
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`
================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp:105
+ 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 use clang-format on these tests, this looks wrong.
================
Comment at: libcxx/utils/data/ignore_format.txt:119
libcxx/include/__algorithm/ranges_find_if_not.h
+libcxx/include/__algorithm/ranges_find_last_if.h
libcxx/include/__algorithm/ranges_for_each.h
----------------
The ignore list is there since we don't want to reformat our entire code base and have merge conflicts for existing patches. New files should always be formatted.
================
Comment at: llvm/utils/gn/secondary/libcxx/include/BUILD.gn:170
+ "__algorithm/ranges_find_last_if.h",
+ "__algorithm/ranges_find_last_if_not.h",
"__algorithm/ranges_for_each.h",
----------------
Typically we don't update the bazel files, there is a post-commit bot that does that.
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