[libcxx-commits] [PATCH] D144394: [libc++] Forward to std::{, w}memchr in std::find

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Mostly LGTM but I'd like to see again before merging, especially after splitting up the test changes.



================
Comment at: libcxx/benchmarks/algorithms/find.bench.cpp:21
+  for (auto _ : state) {
+    auto idx  = rng() % vec1.size();
+    vec1[idx] = '2';
----------------
We should stop/start the timing to avoid including this bit in the benchmark, since this is calling into `mt199whatever` which is non trivial.

You can use `state.PauseTiming()` and `ResumeTiming()` to do this. You should also check whether that introduces a bunch of noise in the timings, I've noticed that before.


================
Comment at: libcxx/include/__algorithm/find.h:68
+template <class _InputIterator, class _Tp>
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX20 _InputIterator
+find(_InputIterator __first, _InputIterator __last, const _Tp& __value) {
----------------
`HIDE_FROM_ABI` while we're at it.


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:118-120
+    char __value_buffer;
+    __builtin_memcpy(&__value_buffer, &__value, sizeof(char));
+    return static_cast<_Tp*>(__builtin_memchr(__str, __value_buffer, __count));
----------------
I would suggest this instead, otherwise it looks like the `memcpy` is trying to tell us something but it's really just a more complicated way to do what's below:

```
return static_cast<_Tp*>(__builtin_memchr(__str, *reinterpret_cast<char*>(&__value), __count));
```


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find.pass.cpp:198
-    {
-      OneWayComparable a[] = { OneWayComparable{true} };
-      auto ret = std::ranges::find(a, a + 1, OneWayComparable{false});
----------------
Could you please split these removals in a different review so we can talk about them separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144394