[V4] Lift meta-functions#112
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:
📝 WalkthroughWalkthroughThis pull request introduces the ChangesLift: Synchronous-to-Async Lifting Facility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/lift.cxx (1)
78-80: 💤 Low valueConsider verifying
return_addris non-null for non-void results.The non-void path dereferences
pkg.return_addrwithout a null check. While the forked path inawait_transform_pkgusesnot_null(pkg.return_addr)(line 145 in promise.cxx), this optimized path doesn't have the same guard.If the invariant that call operations with non-void
Ralways have a valid return address holds by construction, this is fine—but adding an assertion would make the contract explicit and catch violations in debug builds.🛡️ Optional: Add assertion for defensive debugging
} else { - std::move(pkg.args).apply([addr = pkg.return_addr](auto &&fn, auto &&...args) -> void { + auto *addr = pkg.return_addr; + LF_ASSUME(addr != nullptr); + std::move(pkg.args).apply([addr](auto &&fn, auto &&...args) -> void { *addr = std::invoke(LF_FWD(fn), LF_FWD(args)...); }); }🤖 Prompt for 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. In `@src/core/lift.cxx` around lines 78 - 80, The lambda in std::move(pkg.args).apply dereferences pkg.return_addr without checking it; add an explicit debug-time assertion that pkg.return_addr is non-null for non-void results (e.g., before or inside the apply lambda that uses *addr). Reference pkg.return_addr and the apply lambda in lift.cxx; mirror the invariant used in await_transform_pkg/promise.cxx (not_null(pkg.return_addr)) by asserting the pointer (using the project's assert/not_null macro or standard assert) so the contract is explicit in debug builds.
🤖 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.
Nitpick comments:
In `@src/core/lift.cxx`:
- Around line 78-80: The lambda in std::move(pkg.args).apply dereferences
pkg.return_addr without checking it; add an explicit debug-time assertion that
pkg.return_addr is non-null for non-void results (e.g., before or inside the
apply lambda that uses *addr). Reference pkg.return_addr and the apply lambda in
lift.cxx; mirror the invariant used in await_transform_pkg/promise.cxx
(not_null(pkg.return_addr)) by asserting the pointer (using the project's
assert/not_null macro or standard assert) so the contract is explicit in debug
builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f83700a-ff6a-42e7-8d58-3a4d9391441a
📒 Files selected for processing (5)
CMakeLists.txtsrc/core/core.cxxsrc/core/lift.cxxsrc/core/promise.cxxtest/src/lift.cpp
* lift * lift optimization * overload * decay fn to lifted * lifted pass 1 test * doc * await resume * fix semantics * todo * no separate .fn in pkg * assert non-null
Lift a regular function into the task monad
Summary by CodeRabbit