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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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
+{
----------------
Mordante wrote:
> Libc++ should only use __ugly_names, can you rename all `t1` and similar cases to `__t1`?
While you're in the area, please remove all the redundant `const`s from these function headers: `bool cmp_equal(const _T1 __t1...)` is too easily mistaken for (a typo for) `bool cmp_equal(const _T1& __t1...)`. 
So, just write
```
bool cmp_equal(_T1 __t1, _T2 __t2) noexcept
```
Also, if you feel like it, I think it would be more consistent with existing libc++ style to use the parameter names `(_Tp __t, _Up __u)` throughout.


================
Comment at: libcxx/include/utility:339
 {
   return !cmp_equal(__t1, __t2);
 }
----------------
My ADL sense is tingling. This turns out to be OK because `__t1` and `__t2` are guaranteed to be integral types with no associated namespaces; but I think it would still be a peace-of-mind improvement to write `return !_VSTD::cmp_equal(__t1, __t2)` (or rather `return !_VSTD::cmp_equal(__t, __u)`).


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.fail.cpp:27-30
+struct NonEmptyT {
+  static int foo;
+  void func(int);
+};
----------------
This is a confusing name, because `NonEmptyT` is in fact an empty type. Did you mean to give it some non-static data members?

Anyway, the point of this test file seems to be to test that various interesting cases are //not// overload-resolved. So, I recommend testing only the interesting cases. `struct EmptyT {}` already covers the "test a class type" case. If you're looking for more cases to test that are interesting, I suggest

- a class type with an implicit conversion to `int`
- an enum type
- `nullptr_t`
- `__int128_t`

To be clear, I don't think you //should// add any of these (except maybe the enum). But they'd all be much more interesting from the QA perspective than yet another empty class type.


================
Comment at: libcxx/test/std/utilities/utility/intcmp/intcmp.fail.cpp:36-38
+  std::cmp_equal(
+      T(),
+      T()); // expected-error7{{no matching function for call to 'cmp_equal'}}
----------------
Please revert your whitespace changes here. There's nothing wrong with the code on the left, and it's //much// easier to read.

If you'd like to shorten the message, e.g.
```
    std::cmp_equal(T(), T()); // expected-error7{{no matching function}}
```
that'd be fine IMHO. (But unnecessary. There's nothing wrong with the code on the left.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94511