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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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)
----------------
huixie90 wrote:
> 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
Not necessarily. You can `disable_sized_sentinel_for` and are still allowed to have `operator-`. If we forget the check in that case our algorithm wouldn't just be slow, it would be incorrect.


================
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)};
----------------
huixie90 wrote:
> 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`
Doing that would mean that I have to either duplicate the `__search_n_forward_impl` call or the size check. I don't think the added  complexity is justified for that potentially small increase in compilation speed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124079