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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/cwchar:248-249
+
+  return reinterpret_cast<_Tp*>(
+      std::wmemchr(reinterpret_cast<__apply_cv<_Tp, wchar_t>::type*>(__str), __char, __count));
 }
----------------
ldionne wrote:
> For the non-constexpr case, I would do this to make sure that we really never exploit any UB, even if it may seem to be benign right now:
> 
> ```
> if constexpr (std::is_trivial_v<_Tp>) { // we know it's safe to reinterpret cast (at least in C++20 with implicit lifetime types)
>     return reinterpret_cast<_Tp*>(std::wmemchr(reinterpret_cast<__apply_cv<_Tp, wchar_t>::type*>(__str), __char, __count));
> } else {
>     // loop
> }
> ```
> 
> Concretely, this won't change anything since we already only call this function with integral types, however if we were to e.g. specialize `__is_trivially_equality_comparable` for a non-trivial type, then my suggested code would work whereas the original code might be (benign) UB. WDYT?
I went for the "only optimize if we have `__builtin_wmemchr`" option instead. IMO we should try to apply this optimization as broadly as possible, and we can at least heavily influence how clang optimizes `__builtin_wmemchr`. This means that GCC won't optimize this as well, but I think this is OK given that by far most libc++ users use clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144394