This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use the __is_trivially_equality_comparable builtin
ClosedPublic

Authored by philnik on Apr 17 2023, 11:36 AM.

Diff Detail

Event Timeline

philnik created this revision.Apr 17 2023, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 11:36 AM
philnik requested review of this revision.Apr 17 2023, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 11:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 515579.Apr 20 2023, 8:58 PM

Generate files

ldionne requested changes to this revision.May 4 2023, 8:46 AM
ldionne added inline comments.
libcxx/include/__string/constexpr_c_functions.h
44–45

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.

libcxx/include/__type_traits/is_equality_comparable.h
35–38

This needs to be updated.

47–70
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.

libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h
28

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).

31

As discussed.

This revision now requires changes to proceed.May 4 2023, 8:46 AM
philnik updated this revision to Diff 519594.May 4 2023, 12:09 PM
philnik marked 5 inline comments as done.
  • Address comments
  • Try to fix CI
ldionne added inline comments.May 5 2023, 8:33 AM
libcxx/include/__string/char_traits.h
214–215

Can we fix the indentation while we're at it?

215

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".

libcxx/include/__string/constexpr_c_functions.h
55

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.

libcxx/include/__type_traits/is_trivially_lexicographically_comparable.h
27
32
41–44

Nitpick: let's align the comment like

unsigned integer types with sizeof(T) > 1: depending on the endianness [...]
                                           [...]
ldionne accepted this revision.May 5 2023, 1:28 PM

LGTM w/ comments and tests.

libcxx/include/__string/constexpr_c_functions.h
44–45

I don't think you have any tests for this.

55

As discussed, we'd have to get rid of the static_assert and we don't want to do that.

This revision is now accepted and ready to land.May 5 2023, 1:28 PM
philnik updated this revision to Diff 520013.May 5 2023, 6:18 PM
philnik marked 7 inline comments as done.

Address comments

philnik updated this revision to Diff 520131.May 6 2023, 5:08 PM

Try to fix CI

philnik updated this revision to Diff 520202.May 7 2023, 11:20 AM

Try to fix CI