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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM modulo these few last style notes.
I'd also appreciate better indentation in `intcmp.fail.cpp` — don't line-break in the middle of a qualified identifier! — but I know that'll be tedious to fix, and I still volunteer to go fix it later, if need be.



================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp:101-102
+#endif
+      unsigned long long, long long, unsigned long, long, unsigned int, int, unsigned short, short, unsigned char,
+      signed char>
+      types;
----------------
Nit: Please break this line in a sensible way. (Ditto throughout.) I suggest breaking before `unsigned short`.


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater/cmp_greater.pass.cpp:78
+constexpr void test2_impl(const std::tuple<Us...>&) {
+  assert((test_cmp_greater2<T, Us>() && ...));
+}
----------------
Sorry, I may have told you to do this initially, but am now revising it after seeing it in print. The functions `test_cmp_greater1` and `test_cmp_greater2` don't need to return bool because they already assert their own stuff. They should return `void`, and you should use `(test_cmp_greater1<Ts>() , ...);` instead of `assert((test_cmp_greater1<Ts>() && ...));`
(Style note: Prefer `(xs , ...)` in fold-expressions, not `(xs, ...)`, to (subtly/obscurely) emphasize that it's the comma operator being folded and not somehow an ordinary comma-separated list of `xs`.)


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.in_range/in_range.pass.cpp:64-65
+constexpr void test1(const std::tuple<Ts...>&) {
+  assert((test_in_range<Ts>() && ...));
+  assert(test_in_range1());
+}
----------------
As above: `test_in_range` and `test_in_range1` should just return `void`, and this should be a fold-expression over `operator,`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94511