[libcxx-commits] [PATCH] D94511: [libc++] [C++20] [P0586] Implement safe integral comparisons

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/utility:296
+#if _LIBCPP_STD_VER > 17
+#if defined(__cpp_concepts) && __cpp_concepts >= 201811L
+template<class...>
----------------
We recently added a macro to simplify this test. `#if defined(__cpp_concepts) && __cpp_concepts >= 201811L` -> `#if !defined(_LIBCPP_HAS_NO_CONCEPTS)`, or better combine it with the `#if` the line above.


================
Comment at: libcxx/include/utility:300
+
+template<class _Ty1, class _Ty2>
+struct __is_any_of<_Ty1, _Ty2> {
----------------
Can you also switch these to `_Tp` and `_Up`?


================
Comment at: libcxx/include/utility:305
+
+template<class _Ty, class _Ty1, class..._Tyn>
+struct __is_any_of<_Ty, _Ty1, _Tyn...> {
----------------
For consistency with the rest of our code-base please use `_Tp`, `_Up`, and `_Args`.


================
Comment at: libcxx/include/utility:319
+#endif // !_LIBCPP_HAS_NO_UNICODE_CHARS
+                      wchar_t, byte>::value;
+
----------------
Please remove the `byte`.


================
Comment at: libcxx/include/utility:326
+  using T1U = make_unsigned_t<_Tp>;
+  using T2U = make_unsigned_t<_Up>;
+  if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
----------------
I also like to remove the lines with `using ...`. The code @Quuxplusone is easier to understand. Please also adjust `cmp_less` in the same fashion.


================
Comment at: libcxx/include/utility:360
+{
+ return _VSTD::cmp_less(__u, __t);
+}
----------------
Minor nit, but it seems this is indented by 1 space. Please format the patch as described at https://llvm.org/docs/Contributing.html#how-to-submit-a-patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94511