Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Get exception ptr CPO #249
base: main
Are you sure you want to change the base?
Get exception ptr CPO #249
Changes from all commits
f8d2291
cfb4ff0
812e58a
bb8f46b
ec30d89
2461d80
e4cc6f6
aca13cb
532a343
ecad7f0
d326b09
9b57372
8d75bad
a19d14a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 the
std::error_code
can be passed by-value here (it is cheap to copy, and passing by-value allows callers to pass lvalues to this function)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.
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 that there is another case that still needs to be handled in here.
At the moment this code has the following order of preference in which it resolves the implementation of
unifex::set_error(r, err)
:tag_invoke(set_error, r, err)
if that is well-formed; otherwiser.set_error(err)
if that is well-formed; otherwiser.set_error(as_exception_ptr(err))
if that is well-formed; otherwiseset_error(r, err)
call is ill-formedI think that we probably need to have an extra case inserted into here to handle the case where
tag_invoke(set_err, r, err)
is ill-formed buttag_invoke(set_err, r, as_exception_ptr(err))
is well-formed.I think this should probably sit in between cases 2 and 3 above, although it's possible you could argue it should go between cases 1 and 2.
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 could possibly be handled with a two-stage resolution process using a helper.
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.
Are we allowed to use syntax like
if constexpr ( requires { is_this_ill_formed(error); }) {...} else {...}
? Wich is OK in GCC11 and latest MSVC. It would greatly simplify this kind of dispatching stuff.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.
After double-check this syntax does not work on MSVC (I was actualy using clang-cl)
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 not the right way to check for a
.set_error(Error)
member function. Consider thatReceiver::set_error
could be a template. In that case,&Receiver::set_error
is ill-formed. Check how such constraints are checked elsewhere; you'll need to test the validity of the expressionrec.set_error(err)
in a SFINAE context (or with a concept) to get a correct answer.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.
Same thing here about testing for a
.set_error(e)
member function.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.
Usually the nothrow type-trait queries haven't been represented as concepts but rather as boolean type-traits.
I'm not sure exactly why that is, though. Maybe @ericniebler has some thoughts?
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.
Precedence for one. The fact that it's all syntax and no semantics for another. Neither of which are particularly compelling reasons.