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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/cwchar:232
+__constexpr_wmemchr(_Tp* __str, wchar_t __char, size_t __count) {
+  static_assert(sizeof(_Tp) == sizeof(wchar_t) && __is_trivially_equality_comparable<_Tp, _Tp>::value,
+                "Calling wmemchr on non-trivially equality comparable types is unsafe.");
----------------
Should we also assert the we have the same alignment as `wchar_t` here?


================
Comment at: libcxx/include/cwchar:248-249
+
+  return reinterpret_cast<_Tp*>(
+      std::wmemchr(reinterpret_cast<__apply_cv<_Tp, wchar_t>::type*>(__str), __char, __count));
 }
----------------
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?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:10
 // <algorithm>
 
 // template<InputIterator Iter, class T>
----------------
We should also update the tests for `ranges::find`.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:38
 
-int main(int, char**)
-{
-    int ia[] = {0, 1, 2, 3, 4, 5};
-    const unsigned s = sizeof(ia)/sizeof(ia[0]);
-    cpp17_input_iterator<const int*> r = std::find(cpp17_input_iterator<const int*>(ia),
-                                             cpp17_input_iterator<const int*>(ia+s), 3);
-    assert(*r == 3);
-    r = std::find(cpp17_input_iterator<const int*>(ia), cpp17_input_iterator<const int*>(ia+s), 10);
-    assert(r == cpp17_input_iterator<const int*>(ia+s));
-
-#if TEST_STD_VER > 17
-    static_assert(test_constexpr());
+    { // some element matches
+      Iter iter = std::find(Iter(arr), Iter(arr + 10), CompareT(6));
----------------
We should also have a test that checks that if multiple elements match, we return the first one.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:53
+      assert(base(iter) == arr + 10);
+    }
+  }
----------------
Let's also add a test for an empty range.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:57
+
+class Comparable {
+  int i_;
----------------
This shouldn't do the same compare that would be generated anyway, otherwise we are not testing properly that we don't use `memchr`.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find.pass.cpp:59
+  int i_;
+
+public:
----------------
We also need to test with types that have different sizes so we trigger the `memchr` and the `wmemchr` code paths. Right now we don't trigger both with non-integral types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144394