[WIP] get_completion_scheduler query for senders as of project goals #415
Conversation
still need to overload get_completion_scheduler for every possible sender and tests need to be done.
facebook-github-bot
added
the
CLA Signed
|
Currently I have implemented the basic generic get_completion_scheduler query. I need a confirmation from maintainers that everyone agrees with this implementation or some changes need to be done so that I can move ahead with next steps. Thanks |
|
One thing I want to bring to notice... I am using set_done_t instead of set_stopped_t and the reason being to be compatible with other part of codebase that is still using the name done instead of stopped. |
|
I have implemented the get_completion_scheduler query for new_therad_context's scheduler's sender. Still I need to be aware of how to test these. I have added some tests currently, that just checks if query returns the scheduler of same type as expected. I have changed the original get_completion_scheduler query base implementation to not include enable_if and purely work on template specialization. However, there is a little code duplicacy that I would try to reduce... but if anyone want to suggest some options there, It would be so helpful. @LeeHowes @ispeters maintainers feedback is important to get to know, if the implementation is on right path or not. I have done only some initial implementation till now, so it would be easy to change at this stage. |
|
|
||
| template <typename CPO> | ||
| friend auto tag_invoke( | ||
| _get_completion_scheduler::_fn<CPO>, |
There was a problem hiding this comment.
I wanted to write tag_t<get_completion_scheduler<CPO>> instead of _get_completion_scheduler::_fn<CPO>.
However, when I am doing that, it is resulting me to compile error that effectively says that the function declared here is not available to call.
I can confirm that
static_assert(same_as<_get_completion_scheduler::_fn<set_value_t>, tag_t<get_completion_scheduler<set_value_t>>>);
the above static_assert works.
I need some C++ experts suggestions here, that what is exactly getting wrong.
There was a problem hiding this comment.
I've been playing with this and I can't get it to work either. I don't understand enough about the template interactions here to spot what is going wrong. This might benefit from @ericniebler's greater expertise.
There was a problem hiding this comment.
The get_completion_scheduler CPOs must be const or constexpr.
There was a problem hiding this comment.
Hi @ericniebler I have marked get_completion_scheduler to be constexpr variable.... however, still it has the same problem. Am I missing something here?
|
This makes sense to me overall. The code is a bit noisy because you've managed to introduce a lot of formatting changes. |
There was a problem hiding this comment.
@RishabhRD could you revert the formatting changes? This will allow for easier review.
|
Hi everyone, I was unable to work on PR from a long time as my laptop got broken... As @LeeHowes is agreeing with the the approach, I would be continuing with same approach for other files then.... |
@janondrusek I just made a commit regarding reverting formatting changes in new_thread_context.hpp file... If any other file is not formatted well please comment too... Thanks! |
| namespace unifex { | ||
| struct set_value_t {}; | ||
| struct set_error_t {}; | ||
| struct set_done_t {}; |
There was a problem hiding this comment.
These should be the types of the set_value, set_error, and set_done CPOs, not unrelated structs.
There was a problem hiding this comment.
Hi @ericniebler Just made the changes you suggested. Please review it, if I have done the right changes or got something wrong.
| @@ -42,10 +45,10 @@ class _op<Receiver>::type final { | |||
| public: | |||
| template <typename Receiver2> | |||
| explicit type(context* ctx, Receiver2&& r) | |||
| : ctx_(ctx), receiver_((Receiver2&&)r) {} | |||
| : ctx_(ctx), receiver_((Receiver2 &&) r) {} | |||
There was a problem hiding this comment.
@RishabhRD I can still see a lot of unrelated formatting changes made in new_thread_context.hpp.
There was a problem hiding this comment.
@janondrusek yep my mistake... I didn't compare the changes before pushing... I have updated it.
get_completion_scheduler query is demonstrated in P2300R4 and it is also one of the project goals. We also need this for implementing unifex::bulk PR (#354). So, I am starting to implement this.
I am not an expert on this, so I would need continuous pointers on how we want to progress on this.
There are 2 phases of task of this feature I guess:
The latter one seems to be a lot of work to do, so based on maintainers decision we can do one of the following:
I may raise very simple questions on this PR as I am newbie on this (and possibly in C++ too 😅)