[V4] Refactor modules

[ConorWilliams]

Large layout refactor

Summary by CodeRabbit

• New Features

  • Introduced libfork.utils module with deferred execution utilities and refined exception handling.
  • Added libfork.batteries module consolidating adaptors, contexts, and stack implementations.
  • Reorganized into libfork.schedulers for scheduler types.

• Documentation

  • Updated project structure documentation and module organization guides.

• Refactor

  • Restructured module hierarchy for improved organization and clarity.
  • Renamed stack type from geometric to geometric_stack for consistency.
  • Unified exception types using libfork_exception base class.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6f5044a4-c44c-4dae-8d2c-c3fcb24c00ce

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v4-refactor-modules

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/batteries/adaptor_stack.cxx (1)

1-28: ⚠️ Potential issue | 🟡 Minor

Remove the fully commented adaptor_stack.cxx file entirely.

The file is completely dead code with no references in the codebase and no build configuration. Keeping it serves no purpose and introduces unnecessary maintenance overhead. Delete the file.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/batteries/adaptor_stack.cxx` around lines 1 - 28, Delete the dead, fully
commented source file src/batteries/adaptor_stack.cxx from the repo (it only
contains a commented-out adaptor struct in namespace lf::stacks and no active
code such as the template struct adaptor, ckpt, or related functions
push/pop/checkpoint), and ensure no remaining references to the adaptor, ckpt
type, or lf::stacks namespace exist in the codebase or build configuration; if
any incidental references are found, remove or update them accordingly so the
deletion is safe.

🧹 Nitpick comments (3)

src/core/handles.cxx (1)

41-42: Track the inheritance TODO explicitly.

This TODO sits in a public-facing module unit; consider linking it to an issue (or resolving it) to avoid long-lived ambiguity in API shape.

If you want, I can draft a small issue template text for this TODO.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/handles.cxx` around lines 41 - 42, Replace the ambiguous inline TODO
"can we private inherit?" with an explicit tracked action: either (A) resolve it
by changing the public-facing type declared in the same compilation unit to use
private inheritance (adjusting the class declaration, any affected methods, and
public API/tests accordingly) or (B) add a link to a created issue and replace
the TODO with a clear comment referencing that issue (e.g., "TODO: see issue `#NN`
- consider private inheritance and API implications"), plus a one-line
rationale; ensure the chosen change updates any related tests/docs and the
commit message.

src/core/task.cxx (1)

19-22: Consider documenting the purpose of the unused template parameter.

The env struct template takes a worker_context but doesn't use it in the body. This appears intentional as a tag type for context-aware invocation dispatching, but a brief doc comment would clarify this design choice.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/task.cxx` around lines 19 - 22, Add a brief doc comment above the
template struct env explaining that the template parameter worker_context is
intentionally unused and serves as a tag/type-discriminator for context-aware
invocation/dispatch; mention that the explicit constexpr env(key_t) noexcept
constructor exists to accept a key but the template parameter is only for
compile-time tagging (e.g., reference symbols env, worker_context, and the
env(key_t) ctor) so future readers understand the design choice.

src/batteries/contexts.cxx (1)

81-86: dummy_context members are declared but not defined.

This struct declares post, push, pop, and stack but provides no definitions. If this is intentional for compile-time-only concept checking, consider adding a comment. Otherwise, link errors will occur if these are ODR-used.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/batteries/contexts.cxx` around lines 81 - 86, The declarations in
dummy_context (methods post, push, pop, stack) lack definitions and will cause
link errors if ODR-used; either provide appropriate definitions (e.g., inline
no-op implementations or meaningful bodies) for
dummy_context::post(lf::sched_handle<dummy_context>),
dummy_context::push(lf::steal_handle<dummy_context>), dummy_context::pop()
noexcept -> lf::steal_handle<dummy_context>, and dummy_context::stack() noexcept
-> dummy_allocator& so they link, or if they are only for concept checking mark
them explicitly (add a clear comment explaining they are intentionally
undefined) or change them to deleted/defaulted stubs to avoid accidental
ODR-use. Ensure the chosen fix is applied in the same translation unit (make
them inline if in a header) so consumers won’t get linker errors.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Line 144: The doc line describing "utils/                # libfork.utils —
internal utilities (not public API)" is inconsistent because src/libfork.cxx
publicly re-exports libfork.utils; update the description to reflect that
libfork.utils is publicly available (for example: "utils/                #
libfork.utils — shared utility APIs (publicly re-exported)") so the docs match
the actual export behavior from src/libfork.cxx.

In `@docs/structure.md`:
- Line 7: Replace the bare abbreviation "etc" with the punctuated American
English form "etc." wherever it appears (e.g., the occurrence shown in the
diff), updating the docs/structure.md content to include the trailing period;
ensure any instances in the file follow this same change for consistency.

In `@src/batteries/contexts.cxx`:
- Line 63: The pop() method currently declares noexcept unconditionally; change
it to conditionally noexcept by using noexcept(m_container.pop()) so the
exception specification matches the underlying Adaptor's pop() behavior—update
the pop() declaration in the class that defines constexpr auto pop() ->
steal_handle<context_type> to use noexcept(noexcept(m_container.pop())) and
ensure it still returns m_container.pop().

In `@src/core/concepts/context.cxx`:
- Around line 24-30: The worker_context concept declares an unused parameter
sched_handle<T> await; either remove that parameter from the requires clause or
add the missing constraint that uses it—i.e., add a requirement like {
context.post(await) } -> std::same_as<void>; (or noexcept if intended) so the
concept actually constrains post(), or simply drop "sched_handle<T> await" from
the requires parameter list to eliminate the unused symbol; update the concept
worker_context accordingly while keeping existing constraints for
context.push(frame), context.pop(), and context.stack().

In `@src/core/concepts/invocable.cxx`:
- Around line 15-25: The comment on ctx_invoke_t is truncated; update the
comment above the constrained operator() overload to read a complete sentence
such as "Explicitly constrained so overload resolution prefers the constrained
overload," referencing ctx_invoke_t and the template operator()(Fn &&fn, Args
&&...args) that uses requires std::invocable<...>; ensure the explanatory
comment clearly states that the constraint guides overload resolution to select
the constrained operator() when applicable.

In `@src/core/thread_locals.cxx`:
- Line 15: Fix the typo in the top-of-file TODO comment in
src/core/thread_locals.cxx by changing "implictaions" to "implications" in the
TODO ("// TODO: implictaions of thread local on constexpr") so the comment reads
"// TODO: implications of thread local on constexpr"; no code changes beyond
correcting the misspelled word in that comment.

In `@src/utils/utility.cxx`:
- Around line 66-76: The comment says is_sufficiently_aligned supports "fancy
pointers" but the signature uses T* so it only accepts raw pointers; change the
template to accept the pointer type itself (e.g., template<std::size_t Align,
typename Ptr>) and use std::to_address(ptr) or
std::bit_cast<std::uintptr_t>(std::to_address(ptr)) as currently done, keeping
the requires clause on Align; update any uses of T inside the function
accordingly so is_sufficiently_aligned works with fancy pointer types and still
enforces std::has_single_bit(Align).

---

Outside diff comments:
In `@src/batteries/adaptor_stack.cxx`:
- Around line 1-28: Delete the dead, fully commented source file
src/batteries/adaptor_stack.cxx from the repo (it only contains a commented-out
adaptor struct in namespace lf::stacks and no active code such as the template
struct adaptor, ckpt, or related functions push/pop/checkpoint), and ensure no
remaining references to the adaptor, ckpt type, or lf::stacks namespace exist in
the codebase or build configuration; if any incidental references are found,
remove or update them accordingly so the deletion is safe.

---

Nitpick comments:
In `@src/batteries/contexts.cxx`:
- Around line 81-86: The declarations in dummy_context (methods post, push, pop,
stack) lack definitions and will cause link errors if ODR-used; either provide
appropriate definitions (e.g., inline no-op implementations or meaningful
bodies) for dummy_context::post(lf::sched_handle<dummy_context>),
dummy_context::push(lf::steal_handle<dummy_context>), dummy_context::pop()
noexcept -> lf::steal_handle<dummy_context>, and dummy_context::stack() noexcept
-> dummy_allocator& so they link, or if they are only for concept checking mark
them explicitly (add a clear comment explaining they are intentionally
undefined) or change them to deleted/defaulted stubs to avoid accidental
ODR-use. Ensure the chosen fix is applied in the same translation unit (make
them inline if in a header) so consumers won’t get linker errors.

In `@src/core/handles.cxx`:
- Around line 41-42: Replace the ambiguous inline TODO "can we private inherit?"
with an explicit tracked action: either (A) resolve it by changing the
public-facing type declared in the same compilation unit to use private
inheritance (adjusting the class declaration, any affected methods, and public
API/tests accordingly) or (B) add a link to a created issue and replace the TODO
with a clear comment referencing that issue (e.g., "TODO: see issue `#NN` -
consider private inheritance and API implications"), plus a one-line rationale;
ensure the chosen change updates any related tests/docs and the commit message.

In `@src/core/task.cxx`:
- Around line 19-22: Add a brief doc comment above the template struct env
explaining that the template parameter worker_context is intentionally unused
and serves as a tag/type-discriminator for context-aware invocation/dispatch;
mention that the explicit constexpr env(key_t) noexcept constructor exists to
accept a key but the template parameter is only for compile-time tagging (e.g.,
reference symbols env, worker_context, and the env(key_t) ctor) so future
readers understand the design choice.

🪄 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: 50f2ce86-ad46-4b21-8a4c-f1cfcab767c6

📥 Commits

Reviewing files that changed from the base of the PR and between 7856fed and d598345.

📒 Files selected for processing (52)

• AGENTS.md
• CLAUDE.md
• CMakeLists.txt
• benchmark/src/libfork_benchmark/fib/baremetal.cpp
• benchmark/src/libfork_benchmark/fib/fib.hpp
• benchmark/src/libfork_benchmark/fib/libfork.cpp
• docs/structure.md
• src/batteries/adaptor_stack.cxx
• src/batteries/adaptors.cxx
• src/batteries/batteries.cxx
• src/batteries/contexts.cxx
• src/batteries/deque.cxx
• src/batteries/dummy_stack.cxx
• src/batteries/geometric_stack.cxx
• src/core/concepts.cxx
• src/core/concepts/context.cxx
• src/core/concepts/invocable.cxx
• src/core/concepts/scheduler.cxx
• src/core/concepts/stack.cxx
• src/core/core.cxx
• src/core/execute.cxx
• src/core/frame.cxx
• src/core/handles.cxx
• src/core/mono_context.cxx
• src/core/ops.cxx
• src/core/poly_context.cxx
• src/core/promise.cxx
• src/core/receiver.cxx
• src/core/root.cxx
• src/core/schedule.cxx
• src/core/task.cxx
• src/core/thread_locals.cxx
• src/core/utility.cxx
• src/libfork.cxx
• src/schedule/dummy.cxx
• src/schedule/schedule.cxx
• src/schedulers/inline.cxx
• src/schedulers/schedulers.cxx
• src/utils/concepts.cxx
• src/utils/constants.cxx
• src/utils/defer.cxx
• src/utils/exception.cxx
• src/utils/tuple.cxx
• src/utils/utility.cxx
• src/utils/utils.cxx
• test/src/concepts.cpp
• test/src/deque.cpp
• test/src/promise.cpp
• test/src/schedule.cpp
• test/src/stack.cpp
• test/src/tuple.cpp
• test/src/utility.cpp

💤 Files with no reviewable changes (6)

• src/schedule/schedule.cxx
• benchmark/src/libfork_benchmark/fib/fib.hpp
• src/schedule/dummy.cxx
• src/core/mono_context.cxx
• src/core/concepts.cxx
• src/core/utility.cxx