Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ConorWilliams
force-pushed
the
v4-cancellation
branch
7 times, most recently
from
d6489b5 to
865f725
Compare
ConorWilliams
force-pushed
the
v4-cancellation
branch
from
865f725 to
80fea32
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AGENTS.md (1)
145-154:⚠️ Potential issue | 🟠 MajorDocumentation omits linting step that CI still enforces.
The workflow documentation describes the pattern as "Install Dependencies → Configure → Build → Test", but the actual CI workflow (
.github/workflows/lint.yml) also runscodespellandclang-formatchecks with strict enforcement (--Werror). Developers followingAGENTS.mdwon't know to run these checks locally before pushing, potentially leading to unexpected CI failures.📋 Suggested fix to add linting step to the workflow pattern
All workflows follow this pattern: ```yaml - Install Dependencies: brew install ... - Configure: cmake --preset <preset> -DCMAKE_TOOLCHAIN_FILE=<toolchain>.cmake - Build: cmake --build --preset <preset> +- Lint: codespell && clang-format --dry-run --Werror - Test: ctest --preset <preset>Alternatively, if linting is intentionally excluded from developer workflow documentation, consider adding a note explaining that CI performs additional validation steps not required locally. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@AGENTS.mdaround lines 145 - 154, Update the workflow pattern in AGENTS.md
to include the CI linting step (or a note about CI-only checks): add a "Lint"
step listing the exact commands run by .github/workflows/lint.yml (e.g.,
codespell and clang-format --dry-run --Werror) between "Build" and "Test", or
alternatively append a short note that linting is enforced only by CI and not
required locally; reference the workflow pattern block and
.github/workflows/lint.yml when making the change so readers know which checks
to run locally if desired.</details> </blockquote></details> <details> <summary>src/core/promise.cxx (1)</summary><blockquote> `394-461`: _⚠️ Potential issue_ | _🟠 Major_ **Add an assertion to clarify that `handle_stop()` is only called on frames with `kind != root`.** When `stop_requested()` is true at the join race check (line 399), `handle_stop()` calls `final_suspend_leading(self.frame)` at line 460. This is currently safe because `final_suspend_leading` correctly handles `kind == category::call` (returns `parent->handle()` at line 210) before assuming `kind == category::fork` (line 213). However, the assumption that `self.frame->kind != root` is implicit and fragile. While root frames cannot reach this path in practice (they have no exposed API to request stop before join completion), the contract should be explicit. Add an assertion in either `handle_stop()` or `join_awaitable::await_suspend` (before calling `handle_stop()`) to enforce `frame->kind != category::root`, making the precondition visible and preventing future refactors from inadvertently violating it. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/core/promise.cxxaround lines 394 - 461, Add an explicit precondition
check that the frame kind is not root by asserting self.frame->kind !=
category::root; do this either at the top of handle_stop(this join_awaitable
self) or right before calling handle_stop() in join_awaitable::await_suspend
(the branch where stop_requested() is true) so the contract is explicit; use the
same project assertion macro style (e.g. LF_ASSUME or the existing assert macro)
to fail fast if a root frame ever reaches this path and reference the symbols
handle_stop, join_awaitable::await_suspend, self.frame->kind and category::root
when making the change.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (7)
test/src/cancel.cpp (1)
426-429: Make the thread-count loop part of the Catch2 section tree.
tests(scheduler)definesSECTIONs, but Line 426 wraps it in a normal loop with identical section names for eachthr. Catch2 may not create distinct leaf paths per thread count, so 2/3-thread coverage can be skipped or conflated. Wrap each iteration inDYNAMIC_SECTIONor use a generator.Proposed test-structure adjustment
for (std::size_t thr = 1; thr < 4; ++thr) { - TestType scheduler{thr}; - tests(scheduler); + DYNAMIC_SECTION("threads=" << thr) { + TestType scheduler{thr}; + tests(scheduler); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/cancel.cpp` around lines 426 - 429, The for-loop over thr creates duplicate SECTION names because tests(scheduler) defines Catch2 SECTIONs; replace the plain loop with Catch2-managed dynamic sections so each thr produces a distinct test path: for each iteration wrap the call to tests(scheduler) in a DYNAMIC_SECTION (or use a generator) that includes the thr value in its name, referencing the existing TestType, thr loop variable, and tests(scheduler) invocation to ensure 1/2/3-thread runs are treated as separate Catch2 sections.src/core/receiver.cxx (1)
140-148: Nit:token()can beconst, and document the non‑owning lifetime.
token()does not modify*thisand the returnedstop_tokenis non‑owning (it stores a rawstop_source const *intom_state->m_stop). Making itconstallows calls on aconst receiver&and better reflects intent. Also worth documenting that the returned token is only valid while some owner of the underlyingreceiver_stateis alive — afterreceiver::get() &&has released the shared state (line 156) a previously‑obtained token will dangle.Proposed tweak
- constexpr auto token() noexcept -> stop_source::stop_token + constexpr auto token() const noexcept -> stop_source::stop_token requires Stoppable { if (!valid()) { LF_THROW(broken_receiver_error{}); } return get(key(), *m_state).get_stop_token(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/receiver.cxx` around lines 140 - 148, The token() accessor should be marked const and document that the returned stop_token is non‑owning and only valid while the underlying receiver_state is alive; update the declaration of token() (the constexpr auto token() noexcept -> stop_source::stop_token requires Stoppable) to be a const member, add a brief comment noting the stop_token stores a raw pointer into m_state->m_stop and may dangle after receiver::get() && releases the shared state, and ensure the implementation still calls get(key(), *m_state).get_stop_token() without mutating *this.src/core/ops.cxx (1)
188-205: Field nameparent_stop_sourceis misleading — it holds a token, not a source.
parent_stop_sourcehas typestop_source::stop_token, and it's forwarded as theparentargument tochild_scope_ops(stop_source::stop_token parent)at line 196. Renaming it toparent_stop_token(matching theawait_transforminpromise.cxxthat populates it fromself.frame.stop_token) would avoid confusion about ownership.Proposed rename
template <worker_context Context> struct child_scope_awaitable : std::suspend_never { - stop_source::stop_token parent_stop_source; + stop_source::stop_token parent_stop_token; constexpr auto await_resume(this child_scope_awaitable self) -> child_scope_ops<Context> { - return child_scope_ops<Context>{self.parent_stop_source}; + return child_scope_ops<Context>{self.parent_stop_token}; } };And the corresponding designated initializer in
promise.cxxline 563:- return {.parent_stop_source = self.frame.stop_token}; + return {.parent_stop_token = self.frame.stop_token};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ops.cxx` around lines 188 - 205, The field name parent_stop_source in struct child_scope_awaitable is misleading because its type is stop_source::stop_token; rename it to parent_stop_token and update all uses (notably the await_resume return which constructs child_scope_ops<Context>{self.parent_stop_token}) and the designated initializer in promise.cxx that currently sets parent_stop_source from self.frame.stop_token; ensure symbol child_scope_awaitable, await_resume, child_scope_ops(stop_source::stop_token) and the await_transform/populating code in promise.cxx are updated to the new name.src/core/stop.cxx (2)
62-66:constexpron= deleted special members is redundant.Deleted functions are not evaluated, so
constexprhas no effect here; these lines can be simplified.Proposed tweak
- constexpr stop_source(const stop_source &) noexcept = delete; - constexpr stop_source(stop_source &&) noexcept = delete; - constexpr auto operator=(const stop_source &) noexcept -> stop_source & = delete; - constexpr auto operator=(stop_source &&) noexcept -> stop_source & = delete; + stop_source(const stop_source &) = delete; + stop_source(stop_source &&) = delete; + auto operator=(const stop_source &) -> stop_source & = delete; + auto operator=(stop_source &&) -> stop_source & = delete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/stop.cxx` around lines 62 - 66, Remove the redundant constexpr specifier from the deleted special members of stop_source: the copy constructor (stop_source(const stop_source &)), move constructor (stop_source(stop_source &&)), copy assignment operator (operator=(const stop_source &)), and move assignment operator (operator=(stop_source &&)); leave their delete and noexcept/return signatures intact but drop constexpr to simplify the declarations.
109-112: Considerstd::atomic<bool>/std::atomic_flagform_stop.
m_stopis only ever written with the values 0 and 1 and compared against 1; astd::atomic<bool>(orstd::atomic_flagwithtest/test_and_set) is both smaller and more expressive of intent. This also obviates thestd::uint32_tdependency on<cstdint>here. Optional given this is a 4‑byte diff per stop source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/stop.cxx` around lines 109 - 112, Replace the member m_stop in stop_source from std::atomic<std::uint32_t> to a more appropriate atomic boolean type (e.g., std::atomic<bool> or std::atomic_flag) to reflect it only holds 0/1; update all writes (where m_stop is set to 0 or 1) to use false/true (or clear/test_and_set semantics if using std::atomic_flag) and update comparisons against 1 to check for true (or use test()/test_and_set() accordingly); ensure any necessary includes are adjusted (remove <cstdint> if no longer needed and add <atomic> already present) and keep the member name m_stop and the containing stop_source unchanged.src/core/promise.cxx (1)
278-289: Redundant branch:child->stop_tokenequalsparent->frame.stop_tokenwhenStopToken == false.
await_transform_pkgat lines 533–539 already setschild_promise->frame.stop_token = self.frame.stop_tokenwheneverStopToken == false, so the two arms of thisif constexprare observably equivalent — both read the same underlyingstop_source const *. Collapsing them to a singleself.child->stop_requested()check removes a branch and keeps a consistent "check on the child you're about to resume" narrative.Proposed simplification
- // Noop if stopped, must clean-up the child that will never be resumed. - if constexpr (StopToken) { - if (self.child->stop_requested()) [[unlikely]] { - return self.child->handle().destroy(), parent; - } - } else { - if (parent.promise().frame.stop_requested()) [[unlikely]] { - return self.child->handle().destroy(), parent; - } - } + // Noop if stopped, must clean-up the child that will never be resumed. + // child->stop_token was seeded in await_transform_pkg (either pkg.stop_token when + // StopToken=true, or the parent's stop_token otherwise). + if (self.child->stop_requested()) [[unlikely]] { + return self.child->handle().destroy(), parent; + }If the distinction is being preserved intentionally (e.g. to avoid the extra parent-chain hop when the child's token equals the parent's), a brief comment justifying it would help future readers — the current
TODO: test of having a dedicated is_stopped awaitable is quicker(line 272) suggests perf is still being explored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/promise.cxx` around lines 278 - 289, The two branches checking stop_requested() are redundant because await_transform_pkg already copies parent stop_token into child_promise->frame when StopToken == false; collapse the constexpr(StopToken) conditional into a single check that calls self.child->stop_requested() and, if true, performs the same cleanup (self.child->handle().destroy(), return parent). Locate the block in promise.cxx where self.child is inspected and replace the dual-arm if constexpr with the unified check; optionally remove or update the TODO comment about a dedicated is_stopped awaitable if no longer relevant. Also cross-reference await_transform_pkg / child_promise->frame.stop_token to justify the simplification in a short inline comment.src/core/schedule.cxx (1)
62-99: Exception‑safety wording vs. reality — confirm.The docstring states "This function is strongly exception safe." (line 60) but the primary overload has several potentially‑throwing steps outside the
LF_TRYblock:
root_pkg<context_type>(state, std::forward<Fn>(fn), ...)on line 76 (the comment on line 75 even acknowledges it may throw) — if it throws,stateis unchanged and the caller retains ownership, which is fine.get(key(), *state).get_stop_token()on line 84 isnoexcept✓.Only
sch.post(...)on line 91 is guarded. That is actually strong exception safe (no observable side effects on throw), so the comment is correct — but the// Package takes shared ownership of the state; fine if this throws.comment on line 75 reads ambiguous. Minor wording: clarify that on throw the caller‑providedstateis untouched and the function has no side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/schedule.cxx` around lines 62 - 99, Docstring/comment ambiguity: update the comment and/or the function documentation for schedule(Sch&&, std::shared_ptr<receiver_state<R, Stoppable>>, Fn&&, Args&&...) to state that construction of root_pkg (root_pkg<context_type>) may throw but this is strongly exception-safe because the caller-owned state is unchanged on throw, and that only sch.post(sched_handle<context_type>{...}) is the operation that could throw after all local setup; ensure mention that on any throw prior to successful sch.post there are no observable side effects and ownership of the passed state remains with the caller (reference symbols: schedule, root_pkg, receiver_state, sch.post, get(key(), *state).get_stop_token()).🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@benchmark/src/libfork_benchmark/common.hpp`: - Around line 15-23: The function bench_thread_args should guard against std::thread::hardware_concurrency() returning 0; change the local hw computation to use a fallback of 1 (e.g. hw = std::max(1u, std::thread::hardware_concurrency())) so make_args is always invoked for at least one thread; this prevents thread_count<Sch>() and make_scheduler<Sch>() from reading an uninitialized state.range(1) when state.range(1) would otherwise be absent. In `@src/core/promise.cxx`: - Around line 129-138: Add an explicit explanatory comment above the silent exception extraction in the parent->stop_requested() branch inside promise.cxx (and mirror the same explanatory comment at the similar sites in final_suspend_trailing and handle_stop) stating that swallowed exceptions when a stop is requested are an intentional semantic choice (exceptions are suppressed on cancellation/join), describing the rationale and expected behavior (e.g., exceptions are discarded to avoid propagation during cancellation), and reference exception_bit and extract_exception so readers know why extraction is called and discarded; also add/extend a unit test in test/src/cancel.cpp that triggers parent->exception_bit during cancellation to assert the cancellation path exercises the silent-drop behavior. In `@src/core/receiver.cxx`: - Around line 134-148: The public API promises external cancellation via the Stoppable receiver_state but receiver::token() only returns an immutable stop_token and m_state->m_stop is private; add a public receiver::request_stop() (with the same requires Stoppable and behavior as token()) that checks valid() (throwing broken_receiver_error on invalid), and forwards the stop request to the underlying mutable stop_source via the same view/get path (e.g., call view::request_stop() or add request_stop() to the view returned by get(key(), *m_state) which then invokes m_state->m_stop.request_stop()); alternatively, if you prefer keeping mutation out of view, expose a method on receiver_state (when Stoppable) that calls m_stop.request_stop() and have receiver::request_stop() delegate to it — ensure the new API mirrors receiver::token() in safety and noexcept/constraints. In `@src/core/schedule.cxx`: - Around line 49-51: The alias schedule_result_t currently omits the Stoppable template parameter and therefore always expands to receiver<R> (i.e., receiver<R, false>), which mismatches the primary schedule(...) overload that returns receiver<R, Stoppable> deduced from std::shared_ptr<receiver_state<R, Stoppable>>; update schedule_result_t to add a bool Stoppable = false template parameter and forward it into receiver (using invoke_decay_result_t<Fn, Context, Args...> as R) so schedule_result_t<Fn, Context, Args..., Stoppable> = receiver<R, Stoppable>, ensuring compatibility with schedule(), receiver, and receiver_state usage. In `@src/core/stop.cxx`: - Around line 44-112: The code uses non-owning raw pointers (stop_token::m_src and stop_source::m_parent) and deep_stop_requested walks the parent chain without ownership or synchronization, so update the doxygen for stop_token, the stop_source(stop_token) ctor, and related methods (token(), stop_requested(), deep_stop_requested) to explicitly state the lifetime contract: the caller must ensure the associated stop_source and all its ancestor stop_source objects outlive every stop_token and any chained child stop_source created from them (i.e., stop_token is non‑owning and will be UB if the pointed stop_source is destroyed first). In `@test/src/cancel.cpp`: - Around line 10-31: Update the stale test comments in cancel.cpp to use the current stop-token terminology instead of is_cancelled(); specifically, replace references to is_cancelled() and "cancel"/"cancelled" checkpoints with stop_requested()/stop-source semantics and mention stop-token behavior for the same locations referenced (awaitable::await_suspend, parent.promise().frame.stop_requested()/stop source checks, final_suspend_full/final_suspend_trailing parent->stop_requested(), join_awaitable::await_ready forcing suspension when stop requested, join_awaitable::await_suspend handling stop_requested() after winning join race, and handle_cancel behavior when exception_bit is set on a stopped frame). Ensure the comment lines enumerating checkpoints A–F and the other noted ranges (around lines 151-158, 165-170, 205, 267-268, 297-298) consistently use stop_requested()/stop-source wording so the test descriptions map to the current promise.cxx implementation. --- Outside diff comments: In `@AGENTS.md`: - Around line 145-154: Update the workflow pattern in AGENTS.md to include the CI linting step (or a note about CI-only checks): add a "Lint" step listing the exact commands run by .github/workflows/lint.yml (e.g., codespell and clang-format --dry-run --Werror) between "Build" and "Test", or alternatively append a short note that linting is enforced only by CI and not required locally; reference the workflow pattern block and .github/workflows/lint.yml when making the change so readers know which checks to run locally if desired. In `@src/core/promise.cxx`: - Around line 394-461: Add an explicit precondition check that the frame kind is not root by asserting self.frame->kind != category::root; do this either at the top of handle_stop(this join_awaitable self) or right before calling handle_stop() in join_awaitable::await_suspend (the branch where stop_requested() is true) so the contract is explicit; use the same project assertion macro style (e.g. LF_ASSUME or the existing assert macro) to fail fast if a root frame ever reaches this path and reference the symbols handle_stop, join_awaitable::await_suspend, self.frame->kind and category::root when making the change. --- Nitpick comments: In `@src/core/ops.cxx`: - Around line 188-205: The field name parent_stop_source in struct child_scope_awaitable is misleading because its type is stop_source::stop_token; rename it to parent_stop_token and update all uses (notably the await_resume return which constructs child_scope_ops<Context>{self.parent_stop_token}) and the designated initializer in promise.cxx that currently sets parent_stop_source from self.frame.stop_token; ensure symbol child_scope_awaitable, await_resume, child_scope_ops(stop_source::stop_token) and the await_transform/populating code in promise.cxx are updated to the new name. In `@src/core/promise.cxx`: - Around line 278-289: The two branches checking stop_requested() are redundant because await_transform_pkg already copies parent stop_token into child_promise->frame when StopToken == false; collapse the constexpr(StopToken) conditional into a single check that calls self.child->stop_requested() and, if true, performs the same cleanup (self.child->handle().destroy(), return parent). Locate the block in promise.cxx where self.child is inspected and replace the dual-arm if constexpr with the unified check; optionally remove or update the TODO comment about a dedicated is_stopped awaitable if no longer relevant. Also cross-reference await_transform_pkg / child_promise->frame.stop_token to justify the simplification in a short inline comment. In `@src/core/receiver.cxx`: - Around line 140-148: The token() accessor should be marked const and document that the returned stop_token is non‑owning and only valid while the underlying receiver_state is alive; update the declaration of token() (the constexpr auto token() noexcept -> stop_source::stop_token requires Stoppable) to be a const member, add a brief comment noting the stop_token stores a raw pointer into m_state->m_stop and may dangle after receiver::get() && releases the shared state, and ensure the implementation still calls get(key(), *m_state).get_stop_token() without mutating *this. In `@src/core/schedule.cxx`: - Around line 62-99: Docstring/comment ambiguity: update the comment and/or the function documentation for schedule(Sch&&, std::shared_ptr<receiver_state<R, Stoppable>>, Fn&&, Args&&...) to state that construction of root_pkg (root_pkg<context_type>) may throw but this is strongly exception-safe because the caller-owned state is unchanged on throw, and that only sch.post(sched_handle<context_type>{...}) is the operation that could throw after all local setup; ensure mention that on any throw prior to successful sch.post there are no observable side effects and ownership of the passed state remains with the caller (reference symbols: schedule, root_pkg, receiver_state, sch.post, get(key(), *state).get_stop_token()). In `@src/core/stop.cxx`: - Around line 62-66: Remove the redundant constexpr specifier from the deleted special members of stop_source: the copy constructor (stop_source(const stop_source &)), move constructor (stop_source(stop_source &&)), copy assignment operator (operator=(const stop_source &)), and move assignment operator (operator=(stop_source &&)); leave their delete and noexcept/return signatures intact but drop constexpr to simplify the declarations. - Around line 109-112: Replace the member m_stop in stop_source from std::atomic<std::uint32_t> to a more appropriate atomic boolean type (e.g., std::atomic<bool> or std::atomic_flag) to reflect it only holds 0/1; update all writes (where m_stop is set to 0 or 1) to use false/true (or clear/test_and_set semantics if using std::atomic_flag) and update comparisons against 1 to check for true (or use test()/test_and_set() accordingly); ensure any necessary includes are adjusted (remove <cstdint> if no longer needed and add <atomic> already present) and keep the member name m_stop and the containing stop_source unchanged. In `@test/src/cancel.cpp`: - Around line 426-429: The for-loop over thr creates duplicate SECTION names because tests(scheduler) defines Catch2 SECTIONs; replace the plain loop with Catch2-managed dynamic sections so each thr produces a distinct test path: for each iteration wrap the call to tests(scheduler) in a DYNAMIC_SECTION (or use a generator) that includes the thr value in its name, referencing the existing TestType, thr loop variable, and tests(scheduler) invocation to ensure 1/2/3-thread runs are treated as separate Catch2 sections.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
745a5535-91a4-4342-b319-750d52f6f027📒 Files selected for processing (26)
.codespellrcAGENTS.mdCMakeLists.txtbenchmark/CMakeLists.txtbenchmark/external/uts/CMakeLists.txtbenchmark/external/uts/include/uts/rng/brg_sha1.hbenchmark/external/uts/include/uts/rng/brg_types.hbenchmark/external/uts/include/uts/rng/rng.hbenchmark/external/uts/include/uts/uts.hbenchmark/external/uts/src/rng/brg_endian.hbenchmark/external/uts/src/rng/brg_sha1.cbenchmark/external/uts/src/uts.cbenchmark/src/libfork_benchmark/common.hppbenchmark/src/libfork_benchmark/fib/libfork.cppbenchmark/src/libfork_benchmark/uts/libfork.cppbenchmark/src/libfork_benchmark/uts/uts.hppsrc/core/core.cxxsrc/core/frame.cxxsrc/core/ops.cxxsrc/core/promise.cxxsrc/core/receiver.cxxsrc/core/root.cxxsrc/core/schedule.cxxsrc/core/stop.cxxtest/src/cancel.cpptodo.md💤 Files with no reviewable changes (1)
- todo.md
1 participant
Summary by CodeRabbit
New Features
child_scope()API for managing nested task scopesTests
Chores