Page MenuHomePhabricator

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

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

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.Mon, Mar 29, 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.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.
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.Wed, Apr 7, 10:21 AM
curdeius added inline comments.Wed, Apr 7, 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.

Quuxplusone added inline comments.Wed, Apr 7, 11:01 AM
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.Sun, Apr 18, 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.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
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

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 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.Sun, Apr 18, 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.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 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.Mon, Apr 19, 9:53 AM
curdeius accepted this revision.Mon, Apr 19, 1:43 PM

LGTM.

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.