Conversation
📝 WalkthroughWalkthroughAdded type-erasure metadata support to executor_ref via a type_id field in its vtable and public method. Refactored when_all completion signaling to occur after runner self-destruction in final_suspend, eliminating explicit destructor logic. Made type_id() function constexpr. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/capy/ex/executor_ref.hpp (1)
38-70:⚠️ Potential issue | 🔴 CriticalFix constexpr incompatibility with RTTI-enabled builds.
vtable_foris declaredinline constexpr, but it initializestype_idwith&detail::type_id<Ex>(). WhenBOOST_CAPY_NO_RTTIis false (RTTI enabled),type_id<Ex>()is not constexpr and returns the result oftypeid(T), which is a runtime operation. This makes the constexpr initialization invalid and causes compilation failures in RTTI builds.Add a conditional compilation guard or make
vtable_fornon-constexpr when RTTI is enabled:Option 1: Conditional constexpr
`#if` BOOST_CAPY_NO_RTTI template<class Ex> inline constexpr executor_vtable vtable_for = { ... }; `#else` template<class Ex> inline executor_vtable vtable_for = { ... }; `#endif`
🧹 Nitpick comments (1)
include/boost/capy/when_all.hpp (1)
327-327: Remove or justify the redundant comment.
The Line 327 comment restates the immediately following launch logic without explaining rationale. Consider removing it or replacing it with a non-obvious “why” note.Proposed cleanup
- // Launch all tasks concurrentlyAs per coding guidelines: Single-line
//comments within code should be used sparingly and judiciously, explaining the why (not the what or how) when it is non-obvious.
|
An automated preview of the documentation is available at https://130.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-04 03:49:18 UTC |
|
GCOVR code coverage report https://130.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-04 03:53:09 UTC |
Runners now self-destruct in final_suspend before signaling completion. This prevents the race where inline dispatch resumes the parent, destroys when_all_state, and the state destructor tries to destroy still-executing runner handles.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/boost/capy/when_all.hpp`:
- Around line 165-183: The await_suspend method calls caller_ex.dispatch(cont)
inside a noexcept function which can terminate if dispatch throws; fix by either
updating the executor contract to require Executor::dispatch be noexcept (adjust
the executor concept in concept/executor.hpp and document the requirement) or
make await_suspend exception-safe by wrapping the dispatch call in a try/catch
that captures any exception from caller_ex.dispatch(cont) and stores/handles it
via when_all_state (e.g., use caller_ex/caller_ex_.dispatch(cont) inside a try
block and on catch store the exception into state->caller_ex_ or
caller_ex.dispatch error path and ensure no exceptions escape await_suspend);
reference await_suspend, caller_ex, continuation_, and when_all_state to locate
the change.
| void await_suspend(coro h) noexcept | ||
| { | ||
| // Signal completion; last task resumes parent | ||
| return p_->state_->signal_completion(); | ||
| // Extract everything needed for signaling before | ||
| // self-destruction. Inline dispatch may destroy | ||
| // when_all_state, so we can't access members after. | ||
| auto* state = p_->state_; | ||
| auto* counter = &state->remaining_count_; | ||
| auto caller_ex = state->caller_ex_; | ||
| auto cont = state->continuation_; | ||
|
|
||
| // Self-destruct first - state no longer destroys runners | ||
| h.destroy(); | ||
|
|
||
| // Signal completion. If last, dispatch parent. | ||
| // Uses only local copies - safe even if state | ||
| // is destroyed during inline dispatch. | ||
| auto remaining = counter->fetch_sub(1, std::memory_order_acq_rel); | ||
| if(remaining == 1) | ||
| caller_ex.dispatch(cont); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect `coro` ownership semantics
echo "=== Searching for coro definition ==="
rg -n -C3 --type=cpp 'class\s+coro|struct\s+coro|using\s+coro' include/boost/capy/coro.hpp
# Inspect Executor concept requirements for dispatch noexcept
echo -e "\n=== Searching for dispatch in executor concept ==="
rg -n -C3 --type=cpp 'dispatch' include/boost/capy/concept/executor.hppRepository: cppalliance/capy
Length of output: 1691
🏁 Script executed:
#!/bin/bash
# Find when_all_state definition
echo "=== Searching for when_all_state definition ==="
rg -n 'struct\s+when_all_state|class\s+when_all_state' include/boost/capy/when_all.hpp
# Check for runner_handles_ member
echo -e "\n=== Searching for runner_handles_ ==="
rg -n 'runner_handles_' include/boost/capy/when_all.hpp
# Check the destructor and cleanup
echo -e "\n=== Searching for when_all_state destructor ==="
rg -n -A20 '~when_all_state' include/boost/capy/when_all.hppRepository: cppalliance/capy
Length of output: 320
🏁 Script executed:
#!/bin/bash
# Get the when_all_state struct definition
echo "=== when_all_state definition ==="
sed -n '78,150p' include/boost/capy/when_all.hpp
# Also check if there's any cleanup/destruction of handles elsewhere
echo -e "\n=== Checking for handle cleanup patterns ==="
rg -n 'destroy\(\)' include/boost/capy/when_all.hppRepository: cppalliance/capy
Length of output: 2343
🏁 Script executed:
#!/bin/bash
# Get full executor concept documentation
echo "=== Full Executor concept ==="
sed -n '30,100p' include/boost/capy/concept/executor.hppRepository: cppalliance/capy
Length of output: 2432
Verify that Executor::dispatch is non-throwing for noexcept await_suspend.
The Executor concept does not mark dispatch as noexcept (see the syntactic requirements in concept/executor.hpp), but await_suspend calls it in a noexcept context at line 176. If dispatch throws, the program will terminate. Confirm this is intentional or mark dispatch as noexcept in the executor contract.
(Note: The double-destruction concern is not valid—when_all_state explicitly documents that runners self-destruct in final_suspend and expects no destruction during state teardown.)
🤖 Prompt for AI Agents
In `@include/boost/capy/when_all.hpp` around lines 165 - 183, The await_suspend
method calls caller_ex.dispatch(cont) inside a noexcept function which can
terminate if dispatch throws; fix by either updating the executor contract to
require Executor::dispatch be noexcept (adjust the executor concept in
concept/executor.hpp and document the requirement) or make await_suspend
exception-safe by wrapping the dispatch call in a try/catch that captures any
exception from caller_ex.dispatch(cont) and stores/handles it via when_all_state
(e.g., use caller_ex/caller_ex_.dispatch(cont) inside a try block and on catch
store the exception into state->caller_ex_ or caller_ex.dispatch error path and
ensure no exceptions escape await_suspend); reference await_suspend, caller_ex,
continuation_, and when_all_state to locate the change.
2 participants
Summary by CodeRabbit
New Features
Refactor