[libcxx-commits] [PATCH] D103478: [libc++][compare] Implement three_way_comparable[_with] concepts

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

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

@rarutyun: I've marked the places where this is lacking bits you can take from D107036 <https://reviews.llvm.org/D107036>.
Re the testing issue, let's see what @ldionne wants.



================
Comment at: libcxx/include/CMakeLists.txt:102
   __compare/ordering.h
+  __compare/three_way_comparable.h
   __concepts/arithmetic.h
----------------
You'll also need to update `module.modulemap` and run the various Python generator scripts (which you can now do via `ninja libcxx-generate-files` or `make libcxx-generate-files`). Make sure to commit the changed //and added// new files.


================
Comment at: libcxx/include/__compare/three_way_comparable.h:12-16
+#include <__compare/common_comparison_category.h>
+#include <__concepts/equality_comparable.h>
+#include <__concepts/totally_ordered.h>
+#include <__config>
+#include <type_traits>
----------------
You're missing some IWYU here, e.g. `same_as.h`. You need to include these directly, for the benefit of the Modules build; it's not Modules-kosher to rely on the fact that `totally_ordered.h` happens to include `same_as.h`.
Check D107036 for the complete listing.



================
Comment at: libcxx/include/compare:30-36
   // [cmp.common], common comparison category type
   template<class... Ts>
   struct common_comparison_category {
     using type = see below;
   };
   template<class... Ts>
     using common_comparison_category_t = typename common_comparison_category<Ts...>::type;
----------------
Right about here, please update the synopsis comment.
Check D107036 for the right thing.


================
Comment at: libcxx/include/compare:126
 #include <__config>
+#include <__compare/three_way_comparable.h>
 #include <type_traits>
----------------
a-z


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:76-79
+static_assert(
+    !check_three_way_comparable_with<int, int (S::*)() const volatile>());
+static_assert(!check_three_way_comparable_with<
+              int, int (S::*)() const volatile noexcept>());
----------------
cjdb wrote:
> Quuxplusone wrote:
> > Nit: Please avoid taking clang-format's advice in cases like this where it's clearly making the formatting worse.
> > Instead, try treating a clang-format diagnostic as a helpful hint that your lines may be too long. E.g. in this case you don't //really// need to test a `const volatile noexcept` abominable function type, right? This test clearly could be rewritten as `static_assert(!check_three_way_comparable_with<int, int*>())` (or any similar pair of types; the only important thing here is that the expression `a == b` would be ill-formed).
> > Renaming `check_three_way_comparable_with` to simply `check` is another good way to improve the readability of this test file while also helping to make clang-format happy.
> I'm sure @rarutyun's employer doesn't pay them to spend time overriding clang-format.
@rarutyun, you wrote:
> Clang-format suggests incorrect modifications that fail to compile if applied.

For generating the diff to upload, I use `git show -U999 > x.diff` and then "Upload file" into the Phab review. I don't use `arc` at all in my personal workflow. I don't know if this will help you, but at least I personally spend zero time wrestling with `arc`; some amount of time wrestling with `git` instead; and zero time wrestling with `clang-format` because `clang-format` is completely unused in my `git` workflow.

Re Chris's snipe: Don't worry about clang-format. Just write the code the way you'd write it naturally, and then act on Phab feedback (if any) from human libc++ maintainers. clang-format isn't the boss here. :)

Also, note that a massive number of these 230+242 lines can disappear entirely. I tentatively recommend just taking my 101+147 lines of more targeted testing from D107036 (although let's see if @ldionne has a strong preference for these, or wants both, or what). Either way, please delete or fix the misformatting throughout //this// file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478