Page MenuHomePhabricator

[libc++] [C++20] [P0586] Implement safe integral comparisons

Authored by kamleshbhalui on Jan 12 2021, 9:28 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
kamleshbhalui marked 5 inline comments as done.Feb 14 2021, 4:26 PM
kamleshbhalui marked an inline comment as done.
Mordante requested changes to this revision.Feb 16 2021, 9:25 AM

Can you use arc to upload the next diff. This will trigger our CI and we can see whether your patch passes all build nodes.

Also can you address @curdeius request: "Split tests for each added function. Please follow the naming of test files according to the standard paragraphs."


Yes but since it's not an integral it doesn't need to be removed from the set of integrals. I was just concerned libc++ would accept std::byte as an integral.


Please also mark the functions with _LIBCPP_INLINE_VISIBILITY e.g:

template<__is_safe_integral_cmp _T1, __is_safe_integral_cmp _T2>
bool cmp_equal(const _T1 __t1, const _T2 __t2) noexcept

I think it would be nice to also use __is_safe_integral_cmp to properly constrain these functions.

48 ↗(On Diff #323649)

The tests at lines 29-36 all expect to get 10 failures. So if _LIBCPP_NO_HAS_CHAR8_T isn't defined there will be 9 and the test will fail.

58 ↗(On Diff #323649)

There are only tests where std::in_range returns true. I miss test where it fails and test where the template arguments R and T aren't the same.

351 ↗(On Diff #323649)

Can you also test that test1 and test2 pass. Normally we tend to have one constexpr bool test function and call use test(); static_assert(test()); in main. For example `libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp'.

358 ↗(On Diff #323649)

I like these tests!
Maybe add some additional tests where the bitwidth in the first and second tuple differ.

This revision now requires changes to proceed.Feb 16 2021, 9:25 AM

Updated the tests

curdeius retitled this revision from Implement p0586r2 to [libc++] [C++20] [P0586] Implement safe integral comparisons.Mon, Mar 29, 1:05 AM
curdeius edited the summary of this revision. (Show Details)
curdeius added inline comments.
115 ↗(On Diff #333763)

Please test some non-primitive non-enum type.


Please test signed char and unsigned char. This applies to other tests too.

curdeius requested changes to this revision.Mon, Mar 29, 1:07 AM
This revision now requires changes to proceed.Mon, Mar 29, 1:07 AM

Addressed @curdeius comments.

Quuxplusone added inline comments.

My ADL sense is tingling. This turns out to be OK because __t1 and __t2 are guaranteed to be integral types with no associated namespaces; but I think it would still be a peace-of-mind improvement to write return !_VSTD::cmp_equal(__t1, __t2) (or rather return !_VSTD::cmp_equal(__t, __u)).


While you're in the area, please remove all the redundant consts from these function headers: bool cmp_equal(const _T1 __t1...) is too easily mistaken for (a typo for) bool cmp_equal(const _T1& __t1...).
So, just write

bool cmp_equal(_T1 __t1, _T2 __t2) noexcept

Also, if you feel like it, I think it would be more consistent with existing libc++ style to use the parameter names (_Tp __t, _Up __u) throughout.

27–30 ↗(On Diff #335823)

This is a confusing name, because NonEmptyT is in fact an empty type. Did you mean to give it some non-static data members?

Anyway, the point of this test file seems to be to test that various interesting cases are not overload-resolved. So, I recommend testing only the interesting cases. struct EmptyT {} already covers the "test a class type" case. If you're looking for more cases to test that are interesting, I suggest

  • a class type with an implicit conversion to int
  • an enum type
  • nullptr_t
  • __int128_t

To be clear, I don't think you should add any of these (except maybe the enum). But they'd all be much more interesting from the QA perspective than yet another empty class type.

36–38 ↗(On Diff #335823)

Please revert your whitespace changes here. There's nothing wrong with the code on the left, and it's much easier to read.

If you'd like to shorten the message, e.g.

std::cmp_equal(T(), T()); // expected-error7{{no matching function}}

that'd be fine IMHO. (But unnecessary. There's nothing wrong with the code on the left.)

Incorporated @Quuxplusone suggestions.

kamleshbhalui marked 6 inline comments as done.Wed, Apr 7, 10:21 AM
curdeius added inline comments.Wed, Apr 7, 10:31 AM

This doesn't make sense._Tp is unsigned here. You should have swapped t and u.
Cf. the example implementation
But first, you need to add a regression test for this.

I'd like to see some "manual" tests like:

  • max(uint64_t) vs. min(int64_t) (both represented as 0xFF'FF'FF'FF unless I'm mistaken)

And other cases for the different non-trivial conditions of cmp_equal/cmp_less.
Please correct me if they're already present.


Please _Uglify the names.


Please be exhaustive and test less_equal, greater_equal, not_equal too.

Quuxplusone added inline comments.Wed, Apr 7, 11:01 AM

Thanks for renaming T1 to _Tp and T2 to _Up; however, that implies you also need to rename T2U into e.g. _UnsignedUp. Or just get rid of the name; it seems like you use it only once? E.g.

if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
   return __t == __u;
else if constexpr (is_signed_v<_Tp>)
   return (__t >= 0) && (__u == make_unsigned_t<_Tp>(__t));
   return (__u >= 0) && (__t == make_unsigned_t<_Up>(__u));

I'm actually puzzled how this isn't equivalent to simply

if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
   return __t == __u;
   return (__t >= 0) && (__u >= 0) && (__t == __u);

but I don't object to the longer form, especially since it's currently given on cppreference.


It appears that Types1 and Types2 are just the exact same list in a different order.
So I think what you want to do here is use just one list, but do all O(n^2) comparisons:

#ifndef _LIBCPP_HAS_NO_INT128
    __int128_t, __uint128_t,
    unsigned long long, long long, unsigned long, long,
    unsigned int, int, unsigned short, short,
    unsigned char, signed char
> types;
test2(types, types);

preceded by

template <class... Ts>
constexpr void test1(const std::tuple<Ts...>&) {
  assert((test_cmp_equal1<Ts>() && ...));
template <class T, class... Us>
constexpr void test2_impl(const std::tuple<Us...>& utuple) {
    assert((test_cmp_equal2<T, Us>() && ...));
template <class... Ts, class UTuple>
constexpr void test2(const std::tuple<Ts...>&, const UTuple& utuple) {
    (test2_impl<Ts>(utuple) , ...);

You don't need to test uint32_t etc. because those are just library typedefs for the primitive types that you're already testing (e.g. unsigned int or unsigned long).


I think it'd probably make sense to rewrite this as unconditional — lose the if:

assert(std::cmp_greater_equal(T(-1), T(-1)));
assert(std::cmp_greater_equal(-2, tup.min) == std::is_signed_v<T>);
assert(std::cmp_greater_equal(tup.min, -2) == std::is_unsigned_v<T>);
assert(!std::cmp_greater_equal(-2, tup.max));
assert(std::cmp_greater_equal(tup.max, -2));

Spurious newline here.


This style was... not what I wanted. :P I can fix the style post-commit, if need be.

Mordante requested changes to this revision.Sun, Apr 18, 2:45 AM
Mordante added inline comments.

We recently added a macro to simplify this test. #if defined(__cpp_concepts) && __cpp_concepts >= 201811L -> #if !defined(_LIBCPP_HAS_NO_CONCEPTS), or better combine it with the #if the line above.


Can you also switch these to _Tp and _Up?


For consistency with the rest of our code-base please use _Tp, _Up, and _Args.


Please remove the byte.


I also like to remove the lines with using .... The code @Quuxplusone is easier to understand. Please also adjust cmp_less in the same fashion.


Minor nit, but it seems this is indented by 1 space. Please format the patch as described at

This revision now requires changes to proceed.Sun, Apr 18, 2:45 AM
kamleshbhalui marked 12 inline comments as done.

incorporated comments from @Quuxplusone, @Mordante and @curdeius

fixed a typo.

Quuxplusone added inline comments.Sun, Apr 18, 9:11 AM

A type which is already known to be "integral" cannot also be cvref-qualified.
Please replace lines 296–318 with simply

template<class _Tp, class... _Up>
struct _IsSameAsAny : _Or<_IsSame<_Tp, _Up>...> {};

template<class _Tp>
concept __is_safe_integral_cmp = is_integral_v<_Tp> &&
                      !_IsSameAsAny<_Tp, bool, char,
                      char16_t, char32_t,

(Btw I've tested this against your tests and it passes them, so if anyone finds a bug in what I wrote, then you'll need more tests.)


You've got 1-space indents on lines 344, 346, 348. Please make them 2-space.
Ditto lines 325, 327, 329.


These tests also pass when !is_signed_v<T>, so you can remove line 58. (See also my unaddressed comment about the cmp_greater_equal test.)


std::declval<int>() is a verbose way to write 0. Please write std::cmp_greater(0, 0) or std::cmp_greater(42, 42), your choice. (Ditto throughout. And then you can remove some clang-format-introduced linebreaks, too.)


Bogus blank line on line 81.

kamleshbhalui marked 4 inline comments as done.

incorporated @Quuxplusone comments

Quuxplusone accepted this revision as: Quuxplusone.Sun, Apr 18, 6:44 PM

LGTM modulo these few last style notes.
I'd also appreciate better indentation in — don't line-break in the middle of a qualified identifier! — but I know that'll be tedious to fix, and I still volunteer to go fix it later, if need be.


Nit: Please break this line in a sensible way. (Ditto throughout.) I suggest breaking before unsigned short.


Sorry, I may have told you to do this initially, but am now revising it after seeing it in print. The functions test_cmp_greater1 and test_cmp_greater2 don't need to return bool because they already assert their own stuff. They should return void, and you should use (test_cmp_greater1<Ts>() , ...); instead of assert((test_cmp_greater1<Ts>() && ...));
(Style note: Prefer (xs , ...) in fold-expressions, not (xs, ...), to (subtly/obscurely) emphasize that it's the comma operator being folded and not somehow an ordinary comma-separated list of xs.)


As above: test_in_range and test_in_range1 should just return void, and this should be a fold-expression over operator,.

formatted the tests as per @Quuxplusone comments

kamleshbhalui marked 2 inline comments as done.Sun, Apr 18, 10:38 PM


This continues to LGTM.
Can we get a second approver (@curdeius @Mordante @ldionne?), then let's land this, and then I'll update and land D100736 after that.

There are still quite some formatting errors, for example () , instead of (),, please use the instructions here to format the patch

Other than that is seems good now. I still like to see a fresh CI run, to make sure is updated properly.


This should be "defined(__cpp_concepts) && __cpp_concepts >= 201907L", the guard below is correct.
(I'm sure wrote that before, but I can't find it anymore :-()

Make sure to run the script after updating.

Quuxplusone accepted this revision.Mon, Apr 19, 9:39 AM

@Mordante wrote:

There are still quite some formatting errors, for example () , instead of (),, please use the instructions here to format the patch

@Mordante: That's intentional; we write (x , ...) when we're folding over operator,. If clang-format can't tell the difference, that's a bug in clang-format.
(All: Please do not blindly clang-format; that just introduces whitespace errors that someone else will have to fix manually down the road.)

There are still quite some formatting errors, for example () , instead of (),, please use the instructions here to format the patch

It's the case for fold expression, and it is done deliberately after @Quuxplusone suggestions.

Other than that is seems good now. I still like to see a fresh CI run, to make sure is updated properly.

ok, will update this.


kamleshbhalui marked an inline comment as done.Mon, Apr 19, 9:53 AM
curdeius accepted this revision.Mon, Apr 19, 1:43 PM


This revision was not accepted when it landed; it landed in state Needs Review.Mon, Apr 19, 4:26 PM
This revision was automatically updated to reflect the committed changes.