-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
<functional>
: Implement invoke_r
#2019
Conversation
Co-authored-by: frederick-vs-ja <de34@live.cn>
I always forget to check x86...
Also not sure how to workaround the |
|
Sorry, I forgot to update after adding that comment for After some testing, it appears #include <functional>
using namespace std;
int square(int n) {
return n * n;
}
constexpr int square_noexcept(int n) noexcept {
return n * n;
}
static_assert(!noexcept(invoke_r<int>(square, 3)));
static_assert(noexcept(invoke_r<int>(square_noexcept, 3))); This compiles correctly. Adding Edit2: And the answer to that question is yes. Guess I'll file a bug report for "invoke with |
I don't suppose there's a macro I can use to allow the static asserts in And as a final thing before un-drafting the PR (unless the /permissive issue is blocking), should there be coverage for each test with all kinds of callables? Or perhaps one of each callable for at least one test (pretty sure I'm only missing using a data member). |
The compiler intentionally does not provide a macro. We're able to detect it with this technique: STL/tests/std/include/range_algorithm_support.hpp Lines 22 to 41 in 1866b84
Depends on the implementation. If (for whatever reason) you had to write a totally custom implementation, then intensive coverage of callable types would be strongly desirable. However, since you have a nicely layered implementation that calls |
That doesn't appear to be working. They still fail within an Just tested a |
|
Oh right, duh. That makes sense; just going to use normal assert then. Edit: actually how about this confusing condition? static_assert(!noexcept(invoke_r<int>(square, 3)) == !is_permissive, "invoke_r<int>(square, 3) is noexcept"); Edit2: Actually come to think about it that is probably better (minus the double negation) as it would catch when the bug gets fixed. |
(probably should have tested this less confusing version first)
Would it be a good idea to mark |
What's wrong with discarding return value of |
Well having thought about it more probably not great unconditionally. Would be nice to be nodiscard if the callable is also nodiscard though. |
Yeah, too bad we can't detect and "perfectly forward" the nodiscard attribute. |
I've pushed a merge with
|
I'm mirroring this to an MSVC-internal PR. It's totally fine to push changes for code review feedback, but please notify me in that case. |
@@ -267,6 +267,7 @@ | |||
// P1682R3 to_underlying() For Enumerations | |||
// P1951R1 Default Template Arguments For pair's Forwarding Constructor | |||
// P1989R2 Range Constructor For string_view | |||
// P2136R3 invoke_r() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HUGE nit: the paper title does not seem to have the parenthesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - this is intentional, I added the parentheses to the "cleaned-up" title in #1978 to be consistent with the other cleaned-up titles.
Thanks for implementing this pirate-themed invocation! Invoke arrr, matey! 🏴☠️ 😹 🎉 |
Fixes #1978
It's a little light on tests (hence the draft PR) but at what point does it become a test of
invoke
rather thaninvoke_r
?The paper lists 3 things that
invoke_r
can do thatinvoke
can't:I'm not sure how you would test 1, 2 I believe I have covered, and is doing the example from 3 going to be doing anything extra that isn't already covered by the simple
long int
test?And I'm not sure why one test fails for
noexcept
. I assume it's due to/permissive
but then why is__cpp_noexcept_function_type
defined? 😕