Page MenuHomePhabricator

Implement p0586r2
Needs RevisionPublic

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

Details

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
308

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.Sun, Feb 14, 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
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?
Can you also add test with ASSERT_NOEXCEPT to validate the functions are noexcept?

23

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

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

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

addressed @Mordante comments.

kamleshbhalui marked 5 inline comments as done.Sun, Feb 14, 4:26 PM
kamleshbhalui marked an inline comment as done.
Mordante requested changes to this revision.Tue, Feb 16, 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
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!
Maybe add some additional tests where the bitwidth in the first and second tuple differ.

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