[V4] Allocator aware adaptors#100
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-aa-ctx-4
branch
from
47fa125 to
c85ebef
Compare
ConorWilliams
force-pushed
the
v4-aa-ctx-4
branch
from
7aa5902 to
c85ebef
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/batteries/contexts.cxx (1)
49-97:⚠️ Potential issue | 🔴 Critical
std::make_from_tupleagainst abstractpoly_context<Stack>and protected-constructorbase_context<Stack>is ill-formed and prevents instantiation of both piecewise constructors.The two piecewise constructors in
src/batteries/contexts.cxxdelegate throughstd::make_from_tuple, which cannot work for either base:
derived_poly_context(line 55):poly_context<Stack>has pure-virtualpushandpop(src/core/poly_context.cxxlines 50–51), making it abstract.std::make_from_tuple<T>returnsTas a prvalue, and per C++ [class.abstract]/3 an abstract type cannot be a function return type. Instantiation is ill-formed.
mono_context(line 91):base_context<Stack>has a protected default constructor (src/core/poly_context.cxxline 19).std::make_from_tupleis a free function in namespacestdwith no access to protected members, so its internal call tobase_context<Stack>(std::get<I>(args)...)fails access checking.Neither constructor can be instantiated. No existing tests exercise them, which explains why this hasn't surfaced.
Fix: Delegate via an
index_sequencehelper so the base subobject and member are initialized directly, bypassingmake_from_tuple:🛠️ Proposed fix for both classes
template <typename... StackArgs, typename... DequeArgs> requires std::constructible_from<poly_context<Stack>, StackArgs...> && std::constructible_from<Deque, DequeArgs...> constexpr derived_poly_context( std::piecewise_construct_t, std::tuple<StackArgs...> stack_args, - std::tuple<DequeArgs...> - deque_args) noexcept(std::is_nothrow_constructible_v<poly_context<Stack>, StackArgs...> && - std::is_nothrow_constructible_v<Deque, DequeArgs...>) - : poly_context<Stack>(std::make_from_tuple<poly_context<Stack>>(std::move(stack_args))), - m_container(std::make_from_tuple<Deque>(std::move(deque_args))) {} + std::tuple<DequeArgs...> deque_args) // + noexcept(std::is_nothrow_constructible_v<poly_context<Stack>, StackArgs...> && + std::is_nothrow_constructible_v<Deque, DequeArgs...>) + : derived_poly_context(std::move(stack_args), + std::move(deque_args), + std::index_sequence_for<StackArgs...>{}, + std::index_sequence_for<DequeArgs...>{}) {} + + private: + template <typename... StackArgs, typename... DequeArgs, std::size_t... IS, std::size_t... ID> + constexpr derived_poly_context(std::tuple<StackArgs...> &&stack_args, + std::tuple<DequeArgs...> &&deque_args, + std::index_sequence<IS...>, + std::index_sequence<ID...>) // + noexcept(std::is_nothrow_constructible_v<poly_context<Stack>, StackArgs...> && + std::is_nothrow_constructible_v<Deque, DequeArgs...>) + : poly_context<Stack>(std::get<IS>(std::move(stack_args))...), + m_container(std::get<ID>(std::move(deque_args))...) {} + public:Apply the same pattern to
mono_context, replacingpoly_context<Stack>withbase_context<Stack>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/batteries/contexts.cxx` around lines 49 - 97, The piecewise constructors for derived_poly_context and mono_context are ill-formed because std::make_from_tuple tries to construct an abstract poly_context<Stack> and calls a protected base_context<Stack> constructor; replace std::make_from_tuple usage with a tuple-unpacking helper that forwards tuple elements via an index_sequence so you directly initialize the base subobject and m_container (e.g., initialize poly_context<Stack>(std::get<I>(stack_args)...) and m_container(std::get<J>(deque_args)...) using a helper template that expands std::index_sequence), apply the same pattern in mono_context to initialize base_context<Stack> and m_container, and ensure noexcept/require clauses are preserved.
🧹 Nitpick comments (2)
src/batteries/adaptors.cxx (1)
39-47: Consider exposingsize_type(andk_default_capacity) as public.
size_typeandk_default_capacityare declared beforepublic:at line 43, so they are private. However,size_typeappears in the public signature ofadapt_deque(size_type capacity, Allocator const&). Callers can still invoke the constructor via implicit conversion from an integer literal, but they cannot name the type (e.g., to store or forward a capacity value) without resorting todecltype. Mirroring the standard container convention and making these members public would be more ergonomic and matches typical C++ container API style.♻️ Proposed refactor
class adapt_deque { - using size_type = deque<unsafe_steal_handle, Allocator>::size_type; - - static constexpr size_type k_default_capacity = 1024 * 32; - public: + using size_type = deque<unsafe_steal_handle, Allocator>::size_type; + + static constexpr size_type k_default_capacity = 1024 * 32; + constexpr adapt_deque() noexcept(noexcept(Allocator())) : adapt_deque(k_default_capacity, Allocator()) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/batteries/adaptors.cxx` around lines 39 - 47, Move the type alias size_type and the constant k_default_capacity into the public section of adapt_deque so they are accessible to callers; specifically, make size_type and k_default_capacity public (instead of declared before public:) so users can name the capacity type and use the default capacity constant when interacting with the adapt_deque(size_type capacity, Allocator const&) constructor and other APIs.src/schedulers/busy.cxx (1)
40-44: LGTM — plumbing for allocator-aware pool is in place.The added
Alloctemplate parameter and constructor argument, along with the(void)alloc;suppression and theTODO: propagate alloc to m_contexts, m_posted, etc.comment, are a reasonable intermediate step. Just make sure to follow up so callers that pass a non-defaultAllocaren't silently ignored; as written, supplying a custom allocator today has no effect, which could surprise users if this surface is stabilized before the TODO is addressed.Happy to open a tracking issue for propagating
allocintom_contexts,m_posted, and the internalstd::vector<std::jthread>/std::mutex-guarded containers — let me know.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedulers/busy.cxx` around lines 40 - 44, The constructor ignores the provided Alloc (it currently just voids it) so custom allocators are silently dropped; update basic_busy_pool to accept and store the allocator and propagate it into member containers (initialize m_contexts with the allocator, pass alloc into any m_posted, the std::vector<std::jthread>, and any mutex‑guarded containers that support allocator customization), or alternatively keep a member allocator and use it when constructing those members; ensure the Alloc template parameter is actually used in the basic_busy_pool constructor and member initializers for m_contexts, m_posted, and the internal thread/vector/container constructions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.clang-format:
- Line 15: The TODO comment currently reads "update for clan-format 23" which
contains a typo; update that comment to "update for clang-format 23" so the tool
name is correct and searchable (edit the line containing "# TODO: update for
clan-format 23" in .clang-format).
- Line 17: The .clang-format entry uses the enum form "BinPackParameters:
OnePerLine" (BinPackParametersStyle); confirm this is the intended behavior
versus the stricter "AlwaysOnePerLine" or the older boolean form, and either (A)
update CI and contributor toolchain checks to require clang-format >= 20 so the
enum is honored, or (B) revert to a compatible value (boolean form or
"AlwaysOnePerLine") to maintain consistent formatting for contributors on pre-20
toolchains; check/adjust the .clang-format setting "BinPackParameters" and your
CI config that installs/runs clang-format to implement the chosen approach.
In `@src/batteries/adaptors.cxx`:
- Around line 44-48: The default constructor adapt_deque() is incorrectly marked
noexcept(noexcept(Allocator())) while it delegates to
adapt_deque(k_default_capacity, Allocator()) which performs allocations via
m_deque{capacity, alloc} and can throw; change the default ctor to remove the
noexcept specification (or make it conditional on the delegated constructor's
noexcept) so allocation failures propagate instead of calling std::terminate —
update the declaration of adapt_deque() and ensure it delegates to
adapt_deque(size_type, Allocator const&) as before, referencing adapt_deque(),
adapt_deque(size_type capacity, Allocator const&), k_default_capacity, m_deque
and Allocator in your edits.
---
Outside diff comments:
In `@src/batteries/contexts.cxx`:
- Around line 49-97: The piecewise constructors for derived_poly_context and
mono_context are ill-formed because std::make_from_tuple tries to construct an
abstract poly_context<Stack> and calls a protected base_context<Stack>
constructor; replace std::make_from_tuple usage with a tuple-unpacking helper
that forwards tuple elements via an index_sequence so you directly initialize
the base subobject and m_container (e.g., initialize
poly_context<Stack>(std::get<I>(stack_args)...) and
m_container(std::get<J>(deque_args)...) using a helper template that expands
std::index_sequence), apply the same pattern in mono_context to initialize
base_context<Stack> and m_container, and ensure noexcept/require clauses are
preserved.
---
Nitpick comments:
In `@src/batteries/adaptors.cxx`:
- Around line 39-47: Move the type alias size_type and the constant
k_default_capacity into the public section of adapt_deque so they are accessible
to callers; specifically, make size_type and k_default_capacity public (instead
of declared before public:) so users can name the capacity type and use the
default capacity constant when interacting with the adapt_deque(size_type
capacity, Allocator const&) constructor and other APIs.
In `@src/schedulers/busy.cxx`:
- Around line 40-44: The constructor ignores the provided Alloc (it currently
just voids it) so custom allocators are silently dropped; update basic_busy_pool
to accept and store the allocator and propagate it into member containers
(initialize m_contexts with the allocator, pass alloc into any m_posted, the
std::vector<std::jthread>, and any mutex‑guarded containers that support
allocator customization), or alternatively keep a member allocator and use it
when constructing those members; ensure the Alloc template parameter is actually
used in the basic_busy_pool constructor and member initializers for m_contexts,
m_posted, and the internal thread/vector/container constructions.
🪄 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: 00a2503c-0856-45d3-97cb-66b302dda47b
📒 Files selected for processing (27)
.clang-formatCMakeLists.txtbenchmark/src/libfork/fib.cppsrc/batteries/adaptor_stack.cxxsrc/batteries/adaptors.cxxsrc/batteries/contexts.cxxsrc/batteries/deque.cxxsrc/batteries/geometric_stack.cxxsrc/batteries/slab_stack.cxxsrc/core/core.cxxsrc/core/exception.cxxsrc/core/execute.cxxsrc/core/frame.cxxsrc/core/handles.cxxsrc/core/ops.cxxsrc/core/poly_context.cxxsrc/core/promise.cxxsrc/core/receiver.cxxsrc/core/root.cxxsrc/core/schedule.cxxsrc/core/stop.cxxsrc/core/task.cxxsrc/schedulers/busy.cxxsrc/schedulers/inline.cxxsrc/utils/utils.cxxtest/src/cancel.cpptest/src/schedule.cpp
💤 Files with no reviewable changes (1)
- src/utils/utils.cxx
1 participant
Summary by CodeRabbit
New Features
Style
Refactor
Tests