[libcxx-commits] [PATCH] D125300: type_traits: rewrite is_invocable_r in terms of argument passing.

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D125300#3505552 <https://reviews.llvm.org/D125300#3505552>, @jacobsa wrote:

> Dumb question: is there a way to see a list of unresolved comments? All I can
> see how to do is look at _all_ the comments in the diff view, which is hard
> to parse when there are a lot of resolved ones. I'm not 100% sure I addressed
> all of your comments.

I don't know what you are looking at exactly, but on the top right you should see something like `12 / 23 Comments`. You can click on that to go to the next unresolved comment. Right next to that you can also select `Hide "done" Inlines`.



================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:83
+// initialize a CantMove object.
+CantMove cant_move = MakeCantMove();
+
----------------
jacobsa wrote:
> philnik wrote:
> > 
> It's this way on purpose: this is showing that it's possible to use the
> function to initialize a CantMove, i.e. no move constructor is necessary for
> such a result type when invoking the function.
Right. My brain probably went "Oh look, a value initialization with the type spelled out twice; let's just spell it once".


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:91
+// some other type.
+static_assert(!std::is_invocable_r_v<int, decltype(MakeCantMove)>);
----------------
jacobsa wrote:
> philnik wrote:
> > Could you also add all the tests for `is_invocable_r`?
> I assume you don't mean _all_ the tests, right? That seems unnecessary; we're
> a lot more likely to have bugs and inconsistencies in the two sets of tests
> than for the two implementations to be out of sync. I added a test confirming
> that it exists and works. PTAL.
No, I meant //all// the tests. The tests should be able to test all standard library implementations and not just libc++. This means that we shouldn't just test code paths we know exist, but also ones which could very well exist. `is_invocable_r` and `is_invocable_r_v` could be implemented completely differently, so we should test for everything. This also future-proofs the tests. We almost never change any of the tests once they are written because the API is set in stone from now 'till the end of time (or until at some point the standard breaks the API, which hasn't happened in the last 25 years). If anything we almost always only add new tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125300