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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__string/char_traits.h:215
 
   static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 int
+  compare(const char_type* __lhs, const char_type* __rhs, size_t __count) _NOEXCEPT {
----------------
Can we fix the indentation while we're at it?


================
Comment at: libcxx/include/__string/char_traits.h:217
+  compare(const char_type* __lhs, const char_type* __rhs, size_t __count) _NOEXCEPT {
+    if (__libcpp_is_constant_evaluated()) {
+#ifdef _LIBCPP_COMPILER_CLANG_BASED
----------------
We need a comment explaining why we can't use `__constexpr_memcmp` here, this is pretty subtle. You can say something like "`__constexpr_memcmp` requires a trivially lexicographically comparable type, but `char` is not when `char` is a signed type".


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:55
     while (__count != 0) {
       if (*__lhs < *__rhs)
         return -1;
----------------
Is there a reason why we can't simply cast to `unsigned char` here when we do the comparison? That is what `__builtin_memcmp` does in spirit, and it might resolve a bunch of difficulties.


================
Comment at: libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h:26
+
+// A type is_trivially_lexicographically_comparable if the expression `a <=> b` (or their pre-C++20 equvalents) is
+// equivalent to `std::memcmp(&a, &b, sizeof(T))` (with `a` and `b` being of type `T`). There is currently no builtin to
----------------



================
Comment at: libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h:31
+//
+// bools trivially lexicographically comparable, because e.g. false <=> true is valid code. Furthermore, the standard
+// says that [basic.fundamental] "Type bool is a distinct type that has the same object representation, value
----------------



================
Comment at: libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h:40-43
+// unsigned integer types with sizeof(T) > 1: depending on the endianness, the LSB might be the first byte to be
+//   compared. This means that when comparing unsigned(129) and unsigned(2) using memcmp(), the result would be
+//   that 2 > 129.
+//   TODO: Do we want to enable this on big-endian systems?
----------------
Nitpick: let's align the comment like

```
unsigned integer types with sizeof(T) > 1: depending on the endianness [...]
                                           [...]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148553