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

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

Quuxplusone added a comment.

Generally looks good!



================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.equiv/equivalence_relation.pass.cpp:17-23
+static_assert(std::equivalence_relation<bool(int, int), int, int>);
+static_assert(std::equivalence_relation<bool(int, int), double, double>);
+static_assert(std::equivalence_relation<bool(int, double), double, double>);
+
+static_assert(!std::equivalence_relation<bool (*)(), int, double>);
+static_assert(!std::equivalence_relation<bool (*)(int), int, double>);
+static_assert(!std::equivalence_relation<bool (*)(double), int, double>);
----------------
So `bool(...)` is always a relation but `bool (*)(...)` is never a relation? ;)


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.equiv/equivalence_relation.subsumption.pass.cpp:27
+
+static_assert(check_subsumption<int (*)(int, double), int, double>());
+
----------------
IIUC, the point of this test is that `equivalence_relation<F&, T, U>` subsumes `relation<F&, T, U>`.
Is it also mandated that `relation<F&, T, U>` subsumes `equivalence_relation<F&, T, U>`? (I think it is.) If so, let's add a test for that.
I'd like to remove those redundant ampersands at the same time — i.e., change `F&` to `F` everywhere it appears, unless the `&` is significant.

I would also like to see a test that `equivalence_relation<F, T, U>` [subsumes|does not subsume] `equivalence_relation<F, T, T>`.


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.pass.cpp:30
+static_assert(std::predicate<int S::*, S*>);
+static_assert(std::predicate<int (S::*)(), S*>);
+static_assert(!std::predicate<void (S::*)(), S*>);
----------------
Here I do think it's worth testing `std::predicate<int (S::*)(), S&>` as well.


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.pass.cpp:42
+static_assert(std::predicate<Predicate, int, double, char>);
+static_assert(std::predicate<Predicate&, int, double, char>);
+
----------------
Here it's worth testing `!std::predicate<const Predicate, int, double, char>`.



================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.subsumption.pass.cpp:43
+};
+static_assert(!check_subsumption([] { return explicit_bool(); }));
+
----------------
This test confuses me. I think you're saying "a regular-invocable callable whose return value is not convertible to bool, is not a predicate; here is an example." However, that doesn't really have anything to do with subsumption, right? The second overload (lines 21–28) simply drops out of overload resolution entirely because it's not viable. Also, line 41 could be deleted from this test and nothing would change — the existence of that explicit conversion to bool doesn't matter to the test.

I recommend deleting lines 40–43, and just making sure you have the equivalent of
`static_assert(!std::predicate<explicit_bool()>)` in `predicate.pass.cpp`. Arguably even better, add the `__boolean_testable` tests requested by @Mordante, and make sure that `!std::__boolean_testable<explicit_bool>`. (But that would be a libcxx/-only test, so maybe you'd want to keep the test for `!std::predicate<explicit_bool()>` as well, for the benefit of people testing non-libc++ libraries against the libc++ test suite.)


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.relation/relation.subsumption.pass.cpp:24
+template<class F, class T, class U>
+requires std::relation<F&, T, U> && true
+[[nodiscard]] constexpr bool check_subsumption() { return true; }
----------------
Again I'm confused by the 5 redundant `&`s in this file. I recommend removing them (unless that makes the test fail, in which case I'd want to know why).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477