[libcxx-commits] [PATCH] D124079: [libc++] Implement ranges::find_end, ranges::search{, _n}

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision.
huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_search.h:47
+    if constexpr (sized_sentinel_for<_Sent2, _Iter2>) {
+      auto __size2 = ranges::distance(__first2, __last2);
+      if (__size2 == 0)
----------------
nit: I think it might be better to just use `operator-` since you've already checked it is `sized_sentinel_for`. If somehow we forgot to check the `sized_sentinel_for` concept, `operator-` would result in a compile time error if the type does not model it, but `ranges::distance` would result in a runtime linear complexity computation


================
Comment at: libcxx/include/__algorithm/ranges_search_n.h:64-69
+    auto __ret = std::__search_n_forward_impl<_RangesIterOps>(__first, __last,
+                                                              __count,
+                                                              __value,
+                                                              __pred,
+                                                              __proj);
+    return {std::move(__ret.first), std::move(__ret.second)};
----------------
It would be nice if you can restructure the code so that this block of code can be in an `else` block of `if constexpr`, in a way that only one of the two `__search_n_random_access_impl` and `__search_n_forward_impl` would be instantiated. IIUC, with this setup, even if the code path calls `__search_n_random_access_impl`, the `__search_n_forward_impl` has still need to be instantiated because it is not guarded by `if constexpr`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124079/new/

https://reviews.llvm.org/D124079