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
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.
You need to guard the use of char(8|16|32)_t.
used _LIBCPP_NO_HAS_CHAR8_T and _LIBCPP_HAS_NO_UNICODE_CHARS macro to guard char8_t, and char16_t, char32_t.
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.
Is byte considered an integral in libc++?
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.
Libc++ should only use __ugly_names, can you rename all t1 and similar cases to __t1?
Can you add the synopsis for the functions tested?
The tests are user-land code, so please don't use _UglyNames
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> _LIBCPP_INLINE_VISIBILITY constexpr 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.
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.
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.
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'.
I like these tests!