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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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>
----------------
philnik wrote:
> 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).
The function you call has the following signature

```
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 const wchar_t*
__constexpr_wmemchr(const wchar_t* __str, wchar_t __char, size_t __count)
```
So how can this work when `_Tp` is not a `wchar_t`? That requires a `reinterpret_cast` which is not allowed during constant evaluation. What am I missing?



================
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)
----------------
philnik wrote:
> 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.
This is something we have used before. Now it will generate dead code.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:78
+  types::for_each(types::integer_types(), TestTypes<long long>());
+  types::for_each(types::integer_types(), TestTypes<Comparable>());
+
----------------
I really would like to see tests for all types that will use this optimization. Especially to make sure the code is valid during constant evaluation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144394