This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

kamleshbhalui created this revision.Jan 12 2021, 9:28 AM
kamleshbhalui requested review of this revision.Jan 12 2021, 9:28 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 12 2021, 9:28 AM
curdeius requested changes to this revision.EditedJan 12 2021, 10:04 AM
curdeius added a subscriber: curdeius.

The implementation looks okay at the first glance but you need to rework the tests.

  • Test noexcept using ASSERT_NOEXCEPT
  • Add tests in non-constexpr context. Look at other recently added commits and add a test function with asserts that gets called twice, once normally (runtime) and once in static_assert.
  • Split tests for each added function. Please follow the naming of test files according to the standard paragraphs.
  • Add more tests with all standard integer types and extended integer types.
  • Test various values and their combinations: 0 limits::min/max +-1, min/2, max/2 and so on. Test interesting combinations of different types
This revision now requires changes to proceed.Jan 12 2021, 10:04 AM

FWIW, you can take a look at the stub I've started writing at https://github.com/mkurdej/llvm-project/commit/0b7eccea2fbab13e897502c6d44ff958a0d550a9.
You might have a look at tests and other stuff like extended integer types.
It's just a WIP and far from ready though, so use with caution.

libcxx/include/utility
316

You need to guard the use of char(8|16|32)_t.

addressed @curdeius comments.
added tests, few test snippets taken from the link given by @curdeius.

used _LIBCPP_NO_HAS_CHAR8_T and _LIBCPP_HAS_NO_UNICODE_CHARS macro to guard char8_t, and char16_t, char32_t.

note:
I think _LIBCPP_NO_HAS_CHAR8_T should be _LIBCPP_HAS_NO_CHAR8_T since _LIBCPP_NO_HAS_CHAR8_T is used at several places I used it here as well, I can clean it up in
different patch if everyone agrees.

Mordante requested changes to this revision.Feb 14 2021, 12:38 PM
Mordante added a subscriber: Mordante.

I think _LIBCPP_NO_HAS_CHAR8_T should be _LIBCPP_HAS_NO_CHAR8_T since _LIBCPP_NO_HAS_CHAR8_T is used at several places I used it here as well, I can clean it up in
different patch if everyone agrees.

Nice catch! @ldionne Do we want to change the macro or is it considered a part of our API?

I'll had a short look and some remarks, will look in more detail later.

libcxx/include/utility
323

Is byte considered an integral in libc++?

325

Is there a reason no to use template<__is_safe_integral_cmp _T1, __is_safe_integral_cmp _T2>. That can than also be used without the extra requires for cmp_not_equal.

327

Libc++ should only use __ugly_names, can you rename all t1 and similar cases to __t1?

libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp
12 ↗(On Diff #323623)

Can you add the synopsis for the functions tested?
Can you also add test with ASSERT_NOEXCEPT to validate the functions are noexcept?

22 ↗(On Diff #323623)

The tests are user-land code, so please don't use _UglyNames

This revision now requires changes to proceed.Feb 14 2021, 12:38 PM
kamleshbhalui added inline comments.Feb 14 2021, 3:23 PM
libcxx/include/utility
323

no. and the test for this is already in intcmp.fail.cpp.

addressed @Mordante comments.

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

libcxx/include/utility
323

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.

326

Please also mark the functions with _LIBCPP_INLINE_VISIBILITY e.g:

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

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

libcxx/test/std/utilities/utility/intcmp/intcmp.fail.cpp
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.

libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp
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.Mar 29 2021, 1:05 AM
curdeius edited the summary of this revision. (Show Details)
curdeius added inline comments.
libcxx/test/std/utilities/utility/intcmp/intcmp.fail.cpp
115 ↗(On Diff #333763)

Please test some non-primitive non-enum type.

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater/cmp_greater.pass.cpp
103

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

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

Addressed @curdeius comments.

Quuxplusone added inline comments.
libcxx/include/utility
291–292

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

327

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.

libcxx/test/std/utilities/utility/intcmp/intcmp.fail.cpp
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.Apr 7 2021, 10:21 AM
curdeius added inline comments.Apr 7 2021, 10:31 AM
libcxx/include/utility
336

This doesn't make sense._Tp is unsigned here. You should have swapped t and u.
Cf. the example implementation https://raw.githubusercontent.com/fekir/safeintegral/master/safeintegral/safeintegralop_cmp.hpp.
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.

libcxx/include/utility
329

Please _Uglify the names.

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.fail.cpp
53

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

libcxx/include/utility
329–330

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));
else
   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;
else
   return (__t >= 0) && (__u >= 0) && (__t == __u);

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

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp
97–113

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:

std::tuple<
#ifndef _LIBCPP_HAS_NO_INT128
    __int128_t, __uint128_t,
#endif
    unsigned long long, long long, unsigned long, long,
    unsigned int, int, unsigned short, short,
    unsigned char, signed char
> types;
test1(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).

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater_equal/cmp_greater_equal.pass.cpp
57–64

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));
114–115

Spurious newline here.

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.fail.cpp
56–57

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

Mordante requested changes to this revision.Apr 18 2021, 2:45 AM
Mordante added inline comments.
libcxx/include/utility
300

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.

304

Can you also switch these to _Tp and _Up?

309

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

323

Please remove the byte.

330

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.

364

Minor nit, but it seems this is indented by 1 space. Please format the patch as described at https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

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

incorporated comments from @Quuxplusone, @Mordante and @curdeius

fixed a typo.

libcxx/include/utility
315

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,
#ifndef _LIBCPP_NO_HAS_CHAR8_T
                      char8_t,
#endif
#ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
                      char16_t, char32_t,
#endif
                      wchar_t>::value;

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

353

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

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp
59–66

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

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater/cmp_greater.pass.cpp
103

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

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.in_range/in_range.pass.cpp
87

Bogus blank line on line 81.

kamleshbhalui marked 4 inline comments as done.

incorporated @Quuxplusone comments

LGTM modulo these few last style notes.
I'd also appreciate better indentation in intcmp.fail.cpp — 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.

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp
102–103

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

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater/cmp_greater.pass.cpp
79

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

libcxx/test/std/utilities/utility/utility.intcmp/intcmp.in_range/in_range.pass.cpp
65–66

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.Apr 18 2021, 10:38 PM

formatted intcmp.fail.cpp

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 https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

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

libcxx/utils/generate_feature_test_macro_components.py
343–344

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.Apr 19 2021, 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 https://llvm.org/docs/Contributing.html#how-to-submit-a-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 https://llvm.org/docs/Contributing.html#how-to-submit-a-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 generate_feature_test_macro_components.py is updated properly.

ok, will update this.

updated generate_feature_test_macro_components.py.

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

LGTM.

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

@Mordante wrote:

There are still quite some formatting errors, for example () , instead of (),, please use the instructions here to format the patch https://llvm.org/docs/Contributing.html#how-to-submit-a-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.)

I was mainly concerned since a previous iteration had indention errors. I still prefer to leave the formatting to clang-format and be happy with it. But I know you have a different opinion. But since you asked for this formatting change I'm not objecting. So LGTM, even if it already landed.