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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
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>
----------------
Mordante wrote:
> Why not test `is_same_v<_Tp, wchar_t>`?
Because that would disable the optimization for other types that have the same size as `wchar_t`. For these types this optimization is probably UB, but that shouldn't be a problem in practice, since it's on an ABI boundary and no compiler currently optimizes `wmemchr` specifically (and IMO no compiler should ever optimize based on this, since there is no potential gain I can see).


================
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;
----------------
Mordante wrote:
> Does this function work with `char16_t` or `char32_t` (depending on `sizeof(wchar_t)`.)
See my comment above.


================
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)
----------------
Mordante wrote:
> Note that the current way it's written generates unneeded code
This looks quite surprising to me, and I'm not sure it's worth the little bit of time the compiler spends on this.


================
Comment at: libcxx/include/cwchar:240
 #endif
+    for (; __count; --__count) {
+      if (*__str == __char)
----------------
Mordante wrote:
> 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,
I don't think it's worth it. The loop is really simple, and deduplicating this seems like a lot of extra complexity compared to the benefit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144394