[V4] Public-private split

[ConorWilliams]

Summary by CodeRabbit

Release Notes

• New Features

  • Added comprehensive public API documentation covering namespace layout, template concepts, coroutine types, and scheduling primitives.
  • Introduced specialized scheduler type aliases for common context configurations.

• Removed Features

  • Removed experimental dummy allocator and related internal stack implementations.

• Improvements

  • Enhanced compile-time validation of coroutine frame layout consistency.

[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: d2c34d54-39c6-402e-8aa4-45e09ebb7e07

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:

• ✅ Review completed - (🔄 Check again to review again)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v4-pub-priv

---

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: 3

🧹 Nitpick comments (3)

src/core/poly_context.cxx (1)

38-56: Optional: document the default post behavior.

The class-level doc describes poly_context as polymorphic in push/pop/post, but doesn't note that the default post implementation throws post_error. Since this is now a non-pure virtual (so derived classes silently inherit the throwing default), a one-line note here will help consumers avoid surprise runtime exceptions when they forget to override post in a context they intend to schedule onto.

📝 Proposed doc tweak

 /**
  * `@brief` A worker context polymorphic in push/pop/post.
  *
  * This is the canonical/blessed base class in libfork for polymorphic uses
  * cases. Although possible, libfork does not recommend contexts polymorphic
  * in the `.stack` member
+ *
+ * `@note` `push`/`pop` are pure virtual and must be overridden. `post` has a
+ *       default implementation that throws `post_error`; override it in
+ *       derived contexts that should accept externally scheduled tasks.
  */

🤖 Prompt for AI Agents

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

In `@src/core/poly_context.cxx` around lines 38 - 56, Add one-line documentation
to poly_context explaining that the default implementation of virtual void
post(sched_handle<poly_context>) throws post_error; update the class-level
comment (or add a brief comment immediately above the post method) to state that
post has a throwing default implementation so derived classes that intend to be
schedulable must override post to avoid runtime post_error exceptions; reference
the poly_context class and its post method in the sentence.

src/core/promise.cxx (1)

631-665: Minor: consider also asserting on a non-trivial T to catch alignment regressions.

The current set covers void (no return_address) and int (int*). Both have pointer-typed return_address, so any pathological T that bumps the promise alignment past alignof(frame_t) (e.g., an over-aligned type) wouldn't be caught here. Since return_address is always T*, this is unlikely to ever bite, but a single extra assertion using something like std::max_align_t or an over-aligned struct would close that gap cheaply.

🛡️ Optional extra coverage

 static_assert(std::is_standard_layout_v<lf::promise_type<int, unit_context>>);
+
+struct alignas(alignof(std::max_align_t) * 2) overaligned_t {
+  int x;
+};
+static_assert(alignof(lf::promise_type<overaligned_t, unit_context>) == alignof(frame_t));
+static_assert(std::is_standard_layout_v<lf::promise_type<overaligned_t, unit_context>>);

🤖 Prompt for AI Agents

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

In `@src/core/promise.cxx` around lines 631 - 665, Add an additional static_assert
that checks promise alignment for a non-trivial/over-aligned T to catch
alignment regressions: instantiate lf::promise_type with a wider type (e.g.,
std::max_align_t or a custom alignas-overaligned struct) and assert
alignof(lf::promise_type<ThatType, unit_context>) == alignof(frame_t); refer to
the existing symbols frame_t and lf::promise_type<..., unit_context> so the new
assertion sits alongside the current ones for void and int.

src/core/thread_locals.cxx (1)

15-15: Nit: typo in TODO comment.

implictaions → implications.

🤖 Prompt for AI Agents

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

In `@src/core/thread_locals.cxx` at line 15, Update the TODO comment in
thread_locals.cxx to correct the typo: change "implictaions" to "implications"
in the TODO text near the thread-local/constexpr note so the comment reads
"TODO: implications of thread local on constexpr".

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/api.md`:
- Around line 269-272: The API docs list inline_scheduler<Context> but omit the
two new public template aliases mono_inline_scheduler and poly_inline_scheduler;
update the docs in the inline scheduler section to mention both aliases
alongside inline_scheduler<Context>, describe that they are template aliases
providing convenient default context configurations (used in tests/benchmarks),
and ensure their names and short descriptions appear in the same format as the
existing entry so the documented API surface matches the exported symbols
mono_inline_scheduler and poly_inline_scheduler.

In `@src/core/frame.cxx`:
- Around line 14-25: The build fails because frame_type (template
frame_type<Checkpoint>) was de-exported from the frame partition but promise.cxx
still references it via using frame_t = lf::frame_type<unit_checkpoint>;—restore
access by either re-exporting the template (make the enum class category and
template struct frame_type exported in the module interface so lf::frame_type is
visible) or adjust promise.cxx to use an allowed symbol (e.g., add a
forward-declaration of template<class> frame_type or move the definition into
the public header/interface that defines unit_checkpoint) so that
unit_checkpoint and lf::frame_type are resolvable to satisfy the using
declaration.

In `@src/core/thread_locals.cxx`:
- Around line 12-13: Either export the template variable thread_local_context so
other libfork.core partitions can directly reference it, or refactor the call
sites in execute.cxx (6 occurrences), schedule.cxx, and root.cxx to use the
provided accessors get_tls_context() and get_tls_stack() instead; update those
files to replace direct uses of thread_local_context<Context> with calls to the
getters (or add an export declaration for thread_local_context in this
partition) and also correct the TODO typo from "implictaions" to "implications".

---

Nitpick comments:
In `@src/core/poly_context.cxx`:
- Around line 38-56: Add one-line documentation to poly_context explaining that
the default implementation of virtual void post(sched_handle<poly_context>)
throws post_error; update the class-level comment (or add a brief comment
immediately above the post method) to state that post has a throwing default
implementation so derived classes that intend to be schedulable must override
post to avoid runtime post_error exceptions; reference the poly_context class
and its post method in the sentence.

In `@src/core/promise.cxx`:
- Around line 631-665: Add an additional static_assert that checks promise
alignment for a non-trivial/over-aligned T to catch alignment regressions:
instantiate lf::promise_type with a wider type (e.g., std::max_align_t or a
custom alignas-overaligned struct) and assert alignof(lf::promise_type<ThatType,
unit_context>) == alignof(frame_t); refer to the existing symbols frame_t and
lf::promise_type<..., unit_context> so the new assertion sits alongside the
current ones for void and int.

In `@src/core/thread_locals.cxx`:
- Line 15: Update the TODO comment in thread_locals.cxx to correct the typo:
change "implictaions" to "implications" in the TODO text near the
thread-local/constexpr note so the comment reads "TODO: implications of thread
local on constexpr".

🪄 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: bdfe9678-219a-42d4-ac1c-593af3442f19

📥 Commits

Reviewing files that changed from the base of the PR and between 31ddb63 and dac822b.

📒 Files selected for processing (18)

• CMakeLists.txt
• benchmark/lib/macros.hpp
• benchmark/src/baremetal/fib.cpp
• benchmark/src/libfork/fib.cpp
• docs/api.md
• src/batteries/batteries.cxx
• src/batteries/contexts.cxx
• src/batteries/deque.cxx
• src/batteries/dummy_stack.cxx
• src/core/concepts/context.cxx
• src/core/frame.cxx
• src/core/poly_context.cxx
• src/core/promise.cxx
• src/core/task.cxx
• src/core/thread_locals.cxx
• src/schedulers/inline.cxx
• test/src/concepts.cpp
• test/src/promise.cpp

💤 Files with no reviewable changes (5)

• src/batteries/batteries.cxx
• CMakeLists.txt
• src/batteries/contexts.cxx
• test/src/promise.cpp
• src/batteries/dummy_stack.cxx

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether mono/poly inline scheduler aliases exist in the codebase
# and compare with docs coverage.

set -euo pipefail

echo "== Scheduler symbol definitions/usages =="
rg -n -C2 --iglob '*.{hpp,h,hh,hxx,ipp,cpp,cc,cxx,ixx,cppm,md}' \
  '\b(mono_inline_scheduler|poly_inline_scheduler|inline_scheduler)\b'

echo
echo "== docs/api.md scheduler section =="
rg -n -C3 '\b(mono_inline_scheduler|poly_inline_scheduler|inline_scheduler)\b' docs/api.md

Repository: ConorWilliams/libfork

Length of output: 5391

---

Document the new inline scheduler aliases in the API reference.

Lines 269–272 document inline_scheduler<Context>, but the PR exports two new public template aliases (mono_inline_scheduler and poly_inline_scheduler) that are not listed in the API docs. These aliases provide convenient default context configurations and are actively used in tests and benchmarks. Add them to this section alongside the base template to ensure the documentation matches the exported API surface.

🤖 Prompt for AI Agents

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

In `@docs/api.md` around lines 269 - 272, The API docs list
inline_scheduler<Context> but omit the two new public template aliases
mono_inline_scheduler and poly_inline_scheduler; update the docs in the inline
scheduler section to mention both aliases alongside inline_scheduler<Context>,
describe that they are template aliases providing convenient default context
configurations (used in tests/benchmarks), and ensure their names and short
descriptions appear in the same format as the existing entry so the documented
API surface matches the exported symbols mono_inline_scheduler and
poly_inline_scheduler.