Clean up stdexec-only paths and update affected tests#7123
Conversation
|
Can one of the admins verify this patch? |
7ca8cbe to
a24be1a
Compare
|
@isidorostsa Please have a look |
|
BTW, how does this relate to #6708? Does this PR superseds it? |
|
@guptapratykshh thanks for working on this. Could you please proactively keep an eye on the CI results. Currently there seem to be some compilation issues with this PR. |
this PR is a narrower follow up in the same direction as #6708, updated on top of the current master rather than a wholesale replacement for it. It keeps the stdexec-only cleanup goal, but its scope here is mainly removing the remaining dead internal paths and fixing the resulting test issues while preserving backward compatibility. |
Thanks for the explanations. Do we still need #6708? What does it have in addition to your PR? |
after merging #6708 into my branch and updating it to work with current master, I do not think we still need #6708 as separate PR |
c180673 to
82b73f8
Compare
3863598 to
62d14bc
Compare
isidorostsa
left a comment
There was a problem hiding this comment.
Thank you for taking on this task! Removing old senders would be helpful
To make it possible to review this please contain unnecessary or pedantic edits as much as possible.
49ba52e to
d94eaf8
Compare
4e3595b to
d8193f7
Compare
63b569e to
32683b8
Compare
Signed-off-by: guptapratykshh <[email protected]>
…exec PR#2035) Signed-off-by: guptapratykshh <[email protected]>
|
@guptapratykshh Thanks so much for your patience with this. This PR is very large, so it is of utmost importance to separate all unrelated changes into their own PRs. Please make sure that the changes to the build system you applied are strictly related to this PR. I think you should not disable unrelated tests, even if those fail on the CIs, for instance. Also, please pay attention to the copilot comments (and not simply resolve them) as usually there is some truth to what it suggest (even if the suggested solution might not be appropriate). |
| test_uninitialized_copy_n_sender( | ||
| hpx::launch::async, par(task), IteratorTag()); | ||
| test_uninitialized_copy_n_sender( | ||
| hpx::launch::async, par_unseq(task), IteratorTag()); |
There was a problem hiding this comment.
Why did you remove these test cases?
There was a problem hiding this comment.
These tests are still removed. Will you re-add them?
There was a problem hiding this comment.
those test cases were re-added in commit 974334220d
| test_uninitialized_copy_n_exception_sender( | ||
| hpx::launch::async, par(task), IteratorTag()); | ||
| test_uninitialized_copy_n_exception_sender( | ||
| hpx::launch::async, par_unseq(task), IteratorTag()); |
There was a problem hiding this comment.
Same here, why did you remove these testcases. The same applies to more places in other files.
There was a problem hiding this comment.
Same here, these tests are still removed. Will you re-add them? So are many more tests below.
There was a problem hiding this comment.
those test cases were re-added in commit 974334220d
Signed-off-by: guptapratykshh <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 141 out of 160 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
libs/core/execution_base/tests/unit/CMakeLists.txt:1
- This change skips registering all
execution_baseunit tests when using Clang/AppleClang. That’s a large reduction in CI signal for those toolchains and may hide regressions unrelated to the specific warning/ICE being worked around. Prefer targeting only the failing tests (e.g.list(REMOVE_ITEM ...)for known problematic sources) or adding per-target compile options/diagnostic suppression instead of disabling the entire test directory for Clang.
# Copyright (c) 2019 Thomas Heller
Signed-off-by: guptapratykshh <[email protected]>
…leClang<=17 Signed-off-by: guptapratykshh <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 142 out of 161 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmake/templates/std_headers.hpp.in:1
- This change removes the
HPX_HAVE_STDEXECguard and now unconditionally emits#pragma ... diagnostic pop(Clang) and a push+pop block (GCC). For Clang in particular, adiagnostic popwithout a matching earlierpushcommonly triggers warnings/errors. Please ensure the pragmas are correctly paired (either reintroduce the guard, add the missingpush, or avoid emittingpopwhen nopushhas been issued).
// Copyright (c) 2019-@HPX_COPYRIGHT_YEAR@ STE||AR Group
Signed-off-by: guptapratykshh <[email protected]>
hkaiser
left a comment
There was a problem hiding this comment.
Let's go ahead with merging this PR as all tests seem to pass.
|
@guptapratykshh Thank you again for your work on this. I have now merged this PR, however we should keep an eye on how things evolve. |
Proposed Changes
HPX_HAVE_STDEXECguarded cleanup paths and kept the stdexec-only implementation as the single active pathAny background context you want to provide?
Checklist
Not all points below apply to all pull requests.