[PATCH] D136522: [libcxx] patch for implementing ranges::find_last

[Mark de Wever via Phabricator via llvm-commits]

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