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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this, the benchmarks look very nice!



================
Comment at: libcxx/benchmarks/algorithms/find.bench.cpp:9
+
+#include <algorithm>
+#include <benchmark/benchmark.h>
----------------
I really would like the benchmark results you posted in the commit message.


================
Comment at: libcxx/include/__algorithm/find.h:54
+          __enable_if_t<__is_identity<_Proj>::value && __is_trivially_equality_comparable<_Tp, _Up>::value &&
+                            sizeof(_Tp) == sizeof(wchar_t) && alignof(_Tp) >= alignof(wchar_t),
+                        int> = 0>
----------------
Why not test `is_same_v<_Tp, wchar_t>`?


================
Comment at: libcxx/include/__algorithm/find.h:58
+__find_impl(_Tp* __first, _Tp* __last, const _Up& __value, _Proj&) {
+  if (auto __ret = std::__constexpr_wmemchr(__first, __value, __last - __first))
+    return __ret;
----------------
Does this function work with `char16_t` or `char32_t` (depending on `sizeof(wchar_t)`.)


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:73-83
+#if _LIBCPP_STD_VER >= 17 && __has_builtin(__builtin_char_memchr)
+    if constexpr (is_same<_Tp, char>::value)
+      return __builtin_char_memchr(__str, __char, __count);
 #endif
+
+    for (; __count; --__count) {
+      if (*__str == __char)
----------------
Note that the current way it's written generates unneeded code


================
Comment at: libcxx/include/cwchar:240
 #endif
+    for (; __count; --__count) {
+      if (*__str == __char)
----------------
This seems duplicated code from the `char` version, can we combine it in a templated function.
The same issue with not using `else` in `if constexpr` applies here too,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144394