[libcxx-commits] [PATCH] D96477: [libcxx] adds remaining callable concepts

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 2 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.strictweakorder/strict_weak_order.pass.cpp:29
+struct S1 {};
+static_assert(!std::strict_weak_order<int S1::*, S1*, std::nullptr_t>);
+
----------------
Quuxplusone wrote:
> FWIW, I have no idea what this assertion is testing. It's asking whether you can use a pointer-to-data-member as a callable, passing it an `S1*` and `nullptr`, and get back a boolean answer? Isn't the answer obviously "no"?
> 
> Vice versa, it occurs to me that
> ```
> static_assert(std::relation<bool (S::*)(S*), S*, S*>);
> static_assert(std::relation<bool (S::*)(S&), S&, S&>);
> ```
> might be considered non-obvious.
Agreed on both counts, changed in all three files.


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.strictweakorder/strict_weak_order.subsumption.pass.cpp:18-24
+template<class F, class T, class U>
+requires std::relation<F&, T, U>
+[[nodiscard]] constexpr bool check_subsumption() { return false; }
+
+template<class F, class T, class U>
+requires std::strict_weak_order<F&, T, U> && true
+[[nodiscard]] constexpr bool check_subsumption() { return true; }
----------------
Quuxplusone wrote:
> Two instances of `F&` that could be `F`, here.
> (Also, I'd prefer to see you remove the gratuituous clang-format comments; they don't add anything.)
> Two instances of F& that could be F, here.

Thanks for checking, I had a feeling I'd missed a couple.

> (Also, I'd prefer to see you remove the gratuituous clang-format comments; they don't add anything.)

On the contrary; without disabling, the formatting becomes

```
template <class F, class T, class U>
requires std::relation<F, T, U> [[nodiscard]] constexpr bool
check_subsumption() {
  return false;
}

template <class F, class T, class U>
    requires std::strict_weak_order<F, T, U> &&
    true [[nodiscard]] constexpr bool check_subsumption() {
  return true;
}
```
which is not only unreadable: it's inconsistent.

clang-format doesn't properly understand requires-clauses. Once it does, I'll go back and erase all the `clang-format off`/`clang-format on` guards in this corner of testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477