[libcxx-commits] [PATCH] D94511: Implement p0586r2

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/include/utility:318
+template<__is_safe_integral_cmp _T1, __is_safe_integral_cmp _T2>
+constexpr bool cmp_equal(const _T1 __t1, const _T2 __t2) noexcept
+{
----------------
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
```


================
Comment at: libcxx/include/utility:330
+
+template<class _T1, class _T2>
+constexpr bool cmp_not_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.


================
Comment at: libcxx/include/utility:315
+#endif // !_LIBCPP_HAS_NO_UNICODE_CHARS
+                      wchar_t, byte>::value;
+
----------------
kamleshbhalui wrote:
> Mordante wrote:
> > Is `byte` considered an integral in libc++?
> no. and the test for this is already in intcmp.fail.cpp.
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.


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.fail.cpp:48
+#ifndef _LIBCPP_NO_HAS_CHAR8_T
+  test<char8_t>();
+#endif // !_LIBCPP_NO_HAS_CHAR8_T
----------------
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.


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp:58
+    assert(std::in_range<T>(tup.mid - 1));
+    assert(std::in_range<T>(tup.mid + 1));
+    return true;
----------------
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.


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp:351
+
+  static_assert(test1<int8_t, uint8_t, short, unsigned short, int16_t,
+                uint16_t, int, unsigned int, int32_t, uint32_t, long,
----------------
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'.


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp:358
+                >());
+  static_assert(test2(std::tuple<int8_t, uint8_t, short, unsigned short,
+                             int16_t, uint16_t, int, unsigned int, int32_t,
----------------
I like these tests!
Maybe add some additional tests where the bitwidth in the first and second tuple differ.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94511/new/

https://reviews.llvm.org/D94511