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

In D94511#2562349 <https://reviews.llvm.org/D94511#2562349>, @kamleshbhalui wrote:

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



================
Comment at: libcxx/include/utility:315
+#endif // !_LIBCPP_HAS_NO_UNICODE_CHARS
+                      wchar_t, byte>::value;
+
----------------
Is `byte` considered an integral in libc++?


================
Comment at: libcxx/include/utility:317
+
+template<class _T1, class _T2>
+requires(__is_safe_integral_cmp<_T1> && __is_safe_integral_cmp<_T2>)
----------------
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`.


================
Comment at: libcxx/include/utility:319
+requires(__is_safe_integral_cmp<_T1> && __is_safe_integral_cmp<_T2>)
+constexpr bool cmp_equal(const _T1 t1, const _T2 t2) noexcept
+{
----------------
Libc++ should only use __ugly_names, can you rename all `t1` and similar cases to `__t1`?


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp:12
+
+// <utility>
+
----------------
Can you add the synopsis for the functions tested?
Can you also add test with `ASSERT_NOEXCEPT` to validate the functions are `noexcept`?


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.pass.cpp:22
+
+template<typename _Ty>
+struct Tuple {
----------------
The tests are user-land code, so please don't use _UglyNames


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

https://reviews.llvm.org/D94511