Diff Detail
Event Timeline
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.
| libcxx/include/utility | ||
|---|---|---|
| 308 | 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.
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.
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 | ||
|---|---|---|
| 315 | Is byte considered an integral in libc++? | |
| 317 | 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. | |
| 319 | 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 | ||
| 13 | Can you add the synopsis for the functions tested? | |
| 23 | The tests are user-land code, so please don't use _UglyNames | |
| libcxx/include/utility | ||
|---|---|---|
| 315 | no. and the test for this is already in intcmp.fail.cpp. | |
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 | ||
|---|---|---|
| 315 | 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. | |
| 318 | 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 | |
| 330 | 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 | 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 | 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 | 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 | I like these tests! | |