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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D96477#2666439 <https://reviews.llvm.org/D96477#2666439>, @zoecarver wrote:

> This LGTM. I like these tests a lot better :)
>
> Only question: why do we need a separate test file for "subsumption" tests? WDYT about merging those into the "main" test file?

I plan to go back at some point and add subsumption tests for all concepts. I think there are quite a few that would benefit from being in their own source files for brevity's sake, and wanna keep that consistent.
Consider this a pilot?

In D96477#2666733 <https://reviews.llvm.org/D96477#2666733>, @Quuxplusone wrote:

> Line 429/447 still contains a gratuitous whitespace diff.
>
> I suggest fixing that space, then removing the `.subsumption.pass.cpp` tests from this PR, then landing it.
> I think it would be reasonable to take the subsumption tests (with my suggested approach above) into a fresh PR.

I don't see why this is blocking.

We should have a discussion on Discord to address this suggested approach because I don't understand why you consider it to be "better", but it can be landed in a future patch at any time if we agree on that path.
I don't like the idea of removing coverage that shows something is working.



================
Comment at: libcxx/test/std/concepts/concepts.callable/concept.predicate/predicate.subsumption.pass.cpp:17
+
+[[nodiscard]] constexpr bool check_subsumption(std::regular_invocable auto) {
+  return false;
----------------
Quuxplusone wrote:
> I would strongly prefer to see as much of the "cruft" removed from this test as possible, so that we could have a bunch of subsumption tests. The extreme form of that would be to use the `SUBSUMES` macro from https://quuxplusone.github.io/blog/2018/09/23/member-concepts-ii/ . But I think we should actually do more like
> https://godbolt.org/z/51edrGrM8
> ```
> bool model(int, int) { return true; }
> 
> constexpr bool relation_subsumes_predicate(std::relation auto) requires true { return true; }
> constexpr void relation_subsumes_predicate(std::predicate auto) { }
> static_assert( relation_subsumes_predicate(model) );
> ```
> in a single `.compile.fail.cpp` file, so that we can verify easily also when a subsumption relationship does //not// exist.
> 
> If anyone knows a clever way to detect non-subsumption in a .compile.pass.cpp test, ping me in #meta-programming or something. ;)
> I would strongly prefer to see as much of the "cruft" removed from this test as possible, so that we could have a bunch of subsumption tests. 

It's not exactly clear to me what "cruft" you're referring to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96477