[libcxx-commits] [PATCH] D139554: [libc++] Forward to std::memcmp for trivially comparable types in equal

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with feedback applied. In particular, *please* address the test related feedback, since it's not clear to me that you are currently testing the actual changes in the patch.



================
Comment at: libcxx/include/__functional/operations.h:346-347
 
+template <class _Tp>
+struct __is_trivial_equal_to_predicate_for<equal_to<_Tp>, _Tp, _Tp> : true_type {};
+
----------------
`__is_trivial_equal_to_predicate_for<equal_to<>, _Tp, _Tp>` should also be true, right?


================
Comment at: libcxx/include/__type_traits/predicate_traits.h:22
+template <class _Pred, class _Lhs, class _Rhs>
+struct __is_trivial_equal_to_predicate_for : false_type {};
+
----------------
I think `__is_trivial_equality_predicate` is a better name -- when reading `__is_trivial_equal_to_predicate_for`, you can read either `__is_trivial_equalto_predicate_for`, or `__is_trivial_equal_topredicatefor` which keeps throwing me off.


================
Comment at: libcxx/include/cstring:125-143
+  if (__libcpp_is_constant_evaluated()
+#ifdef _LIBCPP_COMPILER_CLANG_BASED
+      && sizeof(_Tp) != 1 && !is_same<_Tp, bool>::value
+#endif
+  ) {
+    while (__count != 0) {
       if (*__lhs < *__rhs)
----------------
I suggest this instead:

```
  if (__libcpp_is_constant_evaluated()) {
    // On Clang, we can use __builtin_memcmp at compile-time for non-bool 1-byte types.
#ifdef _LIBCPP_COMPILER_CLANG_BASED
    if constexpr (sizeof(_Tp) == 1 && !is_same<_Tp, bool>::value) {
      return __builtin_memcmp(__lhs, __rhs, __count);
    }
#endif

    while (...) { ... }
  } else {
    return __builtin_memcmp(__lhs, __rhs, __count);
  }
```


================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_comparable.compile.pass.cpp:37
+
+static_assert(!std::__is_trivially_equality_comparable<float, int>::value, "");
+static_assert(!std::__is_trivially_equality_comparable<double, long long>::value, "");
----------------
Mordante wrote:
> Please add `static_assert(!std::__is_trivially_equality_comparable<float, float>::value, "");` test
You actually don't have `float, float` -- please add it.

Also please add tests for pointers to classes w/ inheritance.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/equal.pass.cpp:15
 //   equal(Iter1 first1, Iter1 last1, Iter2 first2);
 //
 // Introduced in C++14:
----------------
Please make sure to test with an `identity` projection in `ranges::equal`, and also with `std::equal_to<>` and `ranges::equal_to<>`. Otherwise, the actual improvements you make in this patch are not tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139554