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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
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>);
+
----------------
Quuxplusone wrote:
> Here it's worth testing `!std::predicate<const Predicate, int, double, char>`.
> 
Wow, I didn't even realise I left off the `const`.


================
Comment at: libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.subsumption.pass.cpp:43
+};
+static_assert(!check_subsumption([] { return explicit_bool(); }));
+
----------------
Quuxplusone wrote:
> 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.)
> However, that doesn't really have anything to do with subsumption, right? 

Good point, moved.

> 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.

It does, because the "don't directly test the implementation detail" model of testing I follow requires I check that the result of the invocable has a return type that satisfies `convertible_to<bool>` (as opposed to something conceptifying just `is_convertible_v<bool>`). However, ...

> 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.)

...I'm not a fan of adding tests for implementation details, but if we've got a history of doing this in `libcxx/test/libcxx/`, I'll make a separate patch for that before signing off on P0898. @ldionne which would you prefer?


================
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; }
----------------
Quuxplusone wrote:
> 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).
This might be a relic from the Ranges TS. I didn't look too hard, but I can't find any mention of the `&` in standard wording, but this was a thing way back when. I've deleted them across all files (please check this).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477