Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused forwarding fix preserves the intended submission semantics, and the regression test covers the reported defect with no identified blocking issues.
No open issues 🎉
What changed in this PR
This PR fixes execution queue submission so lvalue tasks are copied without modifying the caller’s object, while rvalue tasks retain move semantics.
Changes:
- Replace const-cast forwarding with a private helper that preserves value categories.
- Add a regression test checking that lvalue submission does not move from the original task and produces the expected result.
| File | Description |
|---|---|
test/bthread_execution_queue_unittest.cpp |
Add a copy/move-aware task and lvalue regression test. |
src/bthread/execution_queue_inl.h |
Preserve copy and move semantics through a forwarding helper. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
ExecutionQueue<T>::execute(const T&, ...)casts its const lvalue input toT&and forwards it asT&&. As a result, submitting an lvalue can invokeT's move constructor and unexpectedly modify the caller's object.What is changed and the side effects?
Changed:
const_cast-based forwarding path with an internal forwardinghelper.
it.
Side effects:
Lvalue task submissions now consistently copy the task, as implied by the
const T&API.Rvalue task submissions retain their existing move behavior.
Performance effects:
No expected impact on rvalue submissions. Lvalue submissions now avoid
unintended moves and follow the intended copy path.
Breaking backward compatibility:
No API or ABI changes. Code that unintentionally relied on an lvalue being
moved from will observe the corrected behavior.
Check List: