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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun marked 7 inline comments as done.
rarutyun added inline comments.


================
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:
> > 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.
> > 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.
> 
> See https://reviews.llvm.org/D103478#inline-980152.
Avoiding clang-format. Now everything is written in one line and I agree that it looks better. I would not want to rename `check_three_way_comparable_with` to just `check` because I believe that decreases readability but if you insist I may do that.

I am not sure about reducing tests. Seems like different people have different opinions on that. I would prefer to leave it as is unless you have strong objections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478