[libcxx-commits] [PATCH] D148553: [libc++] Use the __is_trivially_equality_comparable builtin

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:41
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 int
 __constexpr_memcmp(const _Tp* __lhs, const _Up* __rhs, size_t __count) {
+  static_assert(__libcpp_is_trivially_lexicographically_comparable<_Tp, _Up>::value,
----------------
For these two functions, I feel it is useful to clarify that basically since each individual object is trivially lexicographically comparable, we could in theory run `memcmp` on each object in a loop. But since we have multiple such objects laid out contiguously, we might as well run just one big `memcmp` on all the bytes. IMO that insight is kind of useful and it would be useful to record in a comment.

And in fact this also clarifies that we could really be taking any `contiguous_iterator` here and just call `std::to_address` on it before we do the `__builtin_memcmp`. Doing that doesn't buy us anything from the caller though so we should probably not try to generalize this.


================
Comment at: libcxx/include/__type_traits/is_equality_comparable.h:36-37
 // A type is_trivially_equality_comparable if the expression `a == b` is equivalent to `std::memcmp(&a, &b, sizeof(T))`
 // (with `a` and `b` being of type `T`). There is no compiler built-in to check this, so we can only do this for known
 // types. In particular, these are the integral types and raw pointers.
 //
----------------
This needs to be updated.


================
Comment at: libcxx/include/__type_traits/is_equality_comparable.h:46-68
 template <class _Tp, class _Up>
-struct __libcpp_is_trivially_equality_comparable
-    : integral_constant<bool,
-                        __is_equality_comparable<_Tp, _Up>::value && is_integral<_Tp>::value &&
-                            is_same<__remove_cv_t<_Tp>, __remove_cv_t<_Up> >::value> {};
+struct __libcpp_is_trivially_equality_comparable_impl
+    : integral_constant<bool, is_integral<_Tp>::value && is_same<_Tp, _Up>::value> {};
+
+#if __has_builtin(__is_trivially_equality_comparable)
+
+template <class _Tp>
----------------
```
template <class _Tp, class _Up>
struct __libcpp_is_trivially_equality_comparable_impl : false_type {};

template <class _Tp>
struct __libcpp_is_trivially_equality_comparable_impl<_Tp, _Tp>
#if __has_builtin(__is_trivially_equality_comparable)
    : integral_constant<bool, __is_trivially_equality_comparable(_Tp) && __is_equality_comparable<_Tp, _Tp>::value>
#else
    : is_integral<_Tp>
#endif
{};

template <class _Tp>
struct __libcpp_is_trivially_equality_comparable_impl<_Tp*, _Tp*> : true_type {};

template <class _Tp, class _Up>
struct __libcpp_is_trivially_equality_comparable_impl<_Tp*, _Up*> : /* what you have right now */ {};
```

IMO that is a bit easier to follow, but it's otherwise equivalent.


================
Comment at: libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h:27
+template <class _Tp, class _Up>
+struct __libcpp_is_trivially_lexicographically_comparable
+    : integral_constant<bool,
----------------
I would like a comment like we have to `__libcpp_is_trivially_equality_comparable` explaining formally what this trait tells us (e.g. that `a < b` is equivalent to a lexicographical comparison on the bytes of the object???).

I think we also need to discuss a couple of issues that we just touched on during live review, such as endianness and signed vs unsigned (which basically means that the value of the object has to be the exact same thing as its representation). So for example `int(-1)` is less than `int(0)`, but if you compare it with `memcmp`, it compares the bytes themselves and the bytes for `int(-1)` are "greater" than the bytes for `int(0)`.

I think we should also discuss what to do with `bool`. So first, `bool < bool` is allowed, and it is the case that `false < true`. It's kinda weird that it's allowed but it is, so whatever. Now `false` has exactly one possible representation `0x000000000`, and IIRC `true` also has exactly one representation `0x0000001`. If that is correct (IOW if it's UB to have `true` be represented as anything else other than `0x0001`), then we are allowed to assume that anything that comes in as a `bool` has one of these two representations, so we can apply `memcmp` on it and it'll work just like `operator<(bool, bool)`.


================
Comment at: libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h:30
+                        is_same<__remove_cv_t<_Tp>, __remove_cv_t<_Up> >::value && sizeof(_Tp) == 1 &&
+                            is_integral<_Tp>::value> {};
+
----------------
As discussed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148553