-
Notifications
You must be signed in to change notification settings - Fork 294
libcudacxx: guard invocability traits against extended lambdas #6739
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
libcudacxx: guard invocability traits against extended lambdas #6739
Conversation
b1c2267 to
70491f3
Compare
70491f3 to
150d8f6
Compare
miscco
left a comment
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.
I believe it is not correct to add those checks in the generic is_invocable machinery
As the error message points out we need this check in places where we need to query the return type of the invokable.
This is already done in the invoke_result struct above is_invocable
If we only want to invoke the lambda, there is no reason to error out. This is especially true, given that this is a nontrivial source breaking change.
| static_assert(!__detail::__disallow_extended_lambda_invocability_v<_Fn>, | ||
| "Attempt to use an extended __device__ or __host__ __device__ lambda in a context " | ||
| "that requires querying its invocability in host code. Use a named function object or " | ||
| "cuda::proclaim_return_type instead."); |
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.
I think we shouldn't mention cuda::proclaim_return_type, because it won't help here
I don't agree. I had a use case like this: There is no way to express that with The source breaking is a valid point, but in the case we would error out we return a false result, which I would say is even worse |
If you care about the return type you need to query it through |
Address miscco's feedback: keep diagnostics only in invoke_result where the return type is actually queried. The plain is_invocable traits should remain usable for SFINAE without breaking existing code.
|
@miscco Thanks for the feedback. I've removed the static_asserts from the invocability traits and kept the diagnostic only in |
miscco
left a comment
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.
These changes are actually pessimizations of the current code.
We do not want to instantiate types just to access a value that we could have gotten directly.
I believe we should close this
| template <class _Fp, class... _Args> | ||
| inline constexpr bool is_nothrow_invocable_v = | ||
| __nothrow_invocable_r_imp<__is_invocable<_Fp, _Args...>, true, void, _Fp, _Args...>; | ||
| template <class _Ret, class _Fp, class... _Args> | ||
| inline constexpr bool is_nothrow_invocable_r_v = | ||
| __nothrow_invocable_r_imp<__is_invocable_r<_Ret, _Fp, _Args...>, is_void_v<_Ret>, _Ret, _Fp, _Args...>; |
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.
This is actually really bad because it now forces us to instantiate types when we do not want to do that
Summary
invoke_resultto blockis_invocable{,_r}andis_nothrow_invocable{,_r}when host builds introspect extended lambdas__device__and__host__ __device__closures.fail.cpptests that instantiate each trait with extended lambdas to prove the static assertions fireTesting
Fixes #1984.