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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/concepts:253
+  constructible_from<_Tp, const _Tp> && convertible_to<const _Tp, _Tp>;
+
 // [concept.booleantestable]
----------------
Moving `copy_constructible` upward is a severable change you can just land right away without review, if you want to. (IMHO. I mean if it were me, I'd land it. Maybe someone else will chime in to agree/disagree.)


================
Comment at: libcxx/include/concepts:314
+concept strict_weak_order = relation<_Rp, _Tp, _Up>;
+#endif //_LIBCPP_STD_VER > 17 && defined(__cpp_concepts) && __cpp_concepts >= 201811L
 
----------------
You changed this comment, but line 152 implies that you shouldn't have. Also, insert blank line.


================
Comment at: libcxx/test/std/concepts/callable/equiv.compile.pass.cpp:25
+template <class F, class T, class U>
+requires std::equivalence_relation<F, T, U>
+constexpr void ModelsEquivalenceRelation(F, T&&, U&&) noexcept {}
----------------
Please indent the `requires` line by one indent-level (which I guess is 2 spaces, in the style used by this file, blecch).
However, as I say below, I'd rather eliminate these functions altogether...


================
Comment at: libcxx/test/std/concepts/callable/equiv.compile.pass.cpp:29
+template <class F, class T, class U>
+requires(!std::equivalence_relation<F, T, U>)
+constexpr void NotEquivalenceRelation(F, T&&, U&&) noexcept {}
----------------



================
Comment at: libcxx/test/std/concepts/callable/equiv.compile.pass.cpp:51
+    using RegularInvocable::A;
+    NotEquivalenceRelation(&A::F, 0, 0);
+    NotEquivalenceRelation(&A::G, 0, 0);
----------------
I'd prefer to see

    static_assert(!std::equivalence_relation<decltype(&A::F), int, int>);

It's not significantly harder to read or reason about, and I think it's a little simpler when it comes to testing lvalue parameter types (which are woefully absent from the current tests). That is, I'd like to see at least one instance of

    static_assert(!std::equivalence_relation<decltype(&A::F), int&, int&>);

In fact, I'd like to see a test case where

    static_assert(std::equivalence_relation<X, int, int>);
    static_assert(!std::equivalence_relation<X, int&, int&>);

and another test case where

    static_assert(!std::equivalence_relation<X, int, int>);
    static_assert(std::equivalence_relation<X, int&, int&>);


================
Comment at: libcxx/test/std/concepts/callable/equiv.compile.pass.cpp:88
+    /// ModelsEquivalenceRelation(Relation::Greater, 0, 0);
+    /// ModelsEquivalenceRelation(Relation::Greater, 0.0, 0);
+    ModelsEquivalenceRelation(Relation::MaybeRelation(), 0, 0);
----------------
I don't think it's useful to keep these as comments.
What is your understanding of the requirements on libc++ as a conforming implementation of C++20? Is libc++ //permitted// to reject these lines if they were to be uncommented?
If we're not permitted to reject them, then we must accept them, and so you might as well uncomment them permanently. OTOH, if they're supposed to be some sort of "library IFNDR," such that a conforming library might reject them, then it makes sense not to uncomment them (but in that case I'd just delete these lines permanently).


================
Comment at: libcxx/test/std/concepts/callable/functions.h:27-28
+struct Bool {
+  int Value = false;
+  constexpr operator bool() const noexcept { return Value; }
+};
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477