-
-
Notifications
You must be signed in to change notification settings - Fork 509
Remove type alias sender completions support (#6624) #6853
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
base: master
Are you sure you want to change the base?
Remove type alias sender completions support (#6624) #6853
Conversation
|
Can one of the admins verify this patch? |
7976eed to
968c14c
Compare
|
I went through the CI failures, especially |
Yes, reformatting those files using clang-format v20 should fix both, the reported issues from clang-format and inspect. |
|
There are however some build failures as reported by CirclCI, please have a look. |
|
You can ignore the problems reported for the ci/circleci: tests.unit1.container_algorithms step. The compilation errors reported by ci/circleci: tests.unit3 seem to be caused by this PR, though. |
ba53269 to
9fc5e83
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
|
FWIW, the MacOS errors look like a missing #include statement. |
bca150d to
e4c1dc7
Compare
0a00463 to
53bda2a
Compare
examples/1d_stencil/1d_stencil_8.cpp
Outdated
| { | ||
| private: | ||
| typedef hpx::spinlock mutex_type; | ||
| typedef std::mutex mutex_type; |
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.
Using std::mutex from HPX threads is generally not a good idea. This will stop the scheduler that runs on the thread aquiring the lock. This change 'fixes' the issue you are seeing only by coincidence as it changes the overall timing of the task executions.
examples/1d_stencil/1d_stencil_8.cpp
Outdated
| { | ||
| static partition_allocator<double>* alloc = | ||
| new partition_allocator<double>(); | ||
| return *alloc; |
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.
Why did you decide to allocate the allocator on the heap (and where is it free'd)? Wouldn't a simple
static partition_allocator<double> alloc{};
return alloc;
suffice?
| if constexpr (requires { | ||
| typename std::decay_t< | ||
| Sender>::completion_signatures; | ||
| }) |
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.
If detail::has_completion_signatures_v<Sender> causes issues (probably because it attempts to instantiate things that shouldn't be instantiated), wouldn't it be a better solution to fix it's implementation? For instance:
template <typename Sender>
inline constexpr bool has_completion_signatures_v =
requires { typename std::decay_t<Sender>::completion_signatures; };
?
| if (vm.count("c-value")) | ||
| c = vm["c-value"].as<double>(); | ||
| else | ||
| c = 1.0; |
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.
The default value is always 1.0, I don't think the else will be ever executed.
| { | ||
| f.get(); | ||
| } | ||
|
|
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 be simplified by using void_guard (see https://github.com/STEllAR-GROUP/hpx/blob/master/libs/core/type_support/include/hpx/type_support/void_guard.hpp). However I don't see why the separate handling of void would be necessary in the first place. Did you see issues with this?
| "Nt value")("c-value", value<double>()->default_value(1.0), "c value")( | ||
| "dt-value", value<double>()->default_value(1.0), | ||
| "dt value")("dx-value", value<double>()->default_value(1.0), | ||
| "dx value"); |
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.
Could leave this unformatted, please?
| std::conjunction<std::negation<hpx::traits::is_future<Sender>>, | ||
| is_sender<Sender>>, | ||
| std::conjunction<std::negation<hpx::traits::is_future<Senders>>, | ||
| is_sender<Senders>>...>)> |
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.
You may be able to write this as a 'real' requires() clause.
| Env) noexcept -> generate_completion_signatures<Env> | ||
| { | ||
| return {}; | ||
| } |
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.
The implementation shouldn't be necessary as the function will never be actually executed. It is ever only used for type computations (i.e. in unevaluated contexts).
| action_move_semantics_component | ||
| ) | ||
|
|
||
| hpx_set_lib_name(action_move_semantics_component action_move_semantics_test) |
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.
Why is this necessary?
8923653 to
b47f78c
Compare
Signed-off-by: guptapratykshh <[email protected]>
Signed-off-by: guptapratykshh <[email protected]>
Signed-off-by: guptapratykshh <[email protected]>
Signed-off-by: guptapratykshh <[email protected]>
b47f78c to
9306260
Compare
Fixes #6624
Proposed Changes
Any background context you want to provide?
This change aligns with the recent WG21 decision (P3164) to eliminate the option of a nested completion_signatures type alias in favor of relying solely on the get_completion_signatures customization point object.
Checklist
Not all points below apply to all pull requests.