Use std::move_only_function for parallel evel work items - #585
Conversation
std::function requires its callable to be copy-constructible, so work items that need to root a Value had to wrap the move-only RootValue in a std::shared_ptr, costing a heap-allocated control block and atomic reference counting per work item. Since we build with C++23, we can use std::move_only_function instead, allowing work items to capture a RootValue directly. Note that FutureVector::spawn() can no longer brace-initialize the WorkItems vector, since std::initializer_list requires copyable elements. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parallel evaluation work-item type now accepts move-only callables through a portable ChangesParallel evaluation ownership
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libutil/include/nix/util/move-only-function.hh`:
- Around line 42-43: Update the fallback `Impl` constructor in
`move-only-function.hh` to use forwarding-reference deduction while constraining
construction against the decayed callable type, so copyable lvalue callables are
accepted. Preserve move construction for rvalues and initialize the stored `f`
member through the forwarded argument.
🪄 Autofix
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: c7ce8a66-099d-47f6-a501-00be6c01225b
📒 Files selected for processing (3)
src/libexpr/include/nix/expr/parallel-eval.hhsrc/libutil/include/nix/util/meson.buildsrc/libutil/include/nix/util/move-only-function.hh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libexpr/include/nix/expr/parallel-eval.hh
…e_only_function libc++ (as of version 21) doesn't implement std::move_only_function yet, breaking the libcxxStdenv build. Add nix::MoveOnlyFunction, which aliases std::move_only_function when available (per the __cpp_lib_move_only_function feature macro) and otherwise provides a minimal type-erased wrapper that only requires the callable to be move-constructible. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
78339f6 to
539d5f9
Compare
Motivation
This avoids having to put
RootValues instd::shared_ptrs.Context
Summary by CodeRabbit