[V4] No allocation for exceptions#89
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/promise.cxx (1)
218-219: Acknowledge TODO: Exception cancel-safety needs attention.The comment notes potential cancellation leakage. If a frame with a stashed exception is cancelled before
extract_exceptionis called, theuninitialized<std::exception_ptr>storage may never be destroyed, leaking the exception state. Consider tracking this as a follow-up issue.Would you like me to open an issue to track the exception/cancellation interaction?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/promise.cxx` around lines 218 - 219, Add a follow-up issue to track and resolve exception cancel-safety referenced by the TODO in src/core/promise.cxx: note that a stashed exception stored in uninitialized<std::exception_ptr> can leak if a frame is cancelled before extract_exception() runs, describe the potential leak scenario and desired outcomes (e.g., ensure stored exception is destroyed on cancellation, or make storage RAII/cancel-safe), include reproduction steps or unit-test ideas, and link the issue to the TODO comment so it can be picked up later.src/utils/uninitialized.cxx (1)
23-23: Consider simplifying pointer access.The expression
std::bit_cast<T*>(auto{buffer})works becauseauto{buffer}decays the array tostd::byte*, but it's unconventional. A more idiomatic approach:♻️ Simplified pointer access
- auto operator->() noexcept -> T * { return std::launder(std::bit_cast<T *>(auto{buffer})); } + auto operator->() noexcept -> T * { return std::launder(reinterpret_cast<T *>(buffer)); }Note: If
constexprsupport is desired in the future (per the TODO about trivial union), the currentbit_castapproach won't help sincereinterpret_castand pointerbit_castaren'tconstexpranyway—only the trivial union approach would enable that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/uninitialized.cxx` at line 23, The operator-> implementation uses an unconventional std::bit_cast of auto{buffer}; change it to a conventional pointer reinterpretation: return std::launder(reinterpret_cast<T*>(buffer)); so locate operator-> (the auto operator->() noexcept -> T * function) and replace the std::bit_cast(auto{buffer}) expression with a reinterpret_cast to T* of the internal buffer variable; keep std::launder and noexcept as-is and ensure buffer refers to the stored std::byte array used by this uninitialized wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/uninitialized.cxx`:
- Around line 10-37: The buffer is incorrectly fixed to sizeof(std::max_align_t)
which can overflow for T; change the storage to be sized and aligned for T by
replacing the private member buffer with "alignas(alignof(T)) std::byte
buffer[sizeof(T)];" and add a compile-time check (e.g., static_assert(sizeof(T)
>= 1, "T must be complete") or static_assert(std::is_trivially_destructible_v<T>
|| true) as needed) to ensure T is a complete type; update usages (operator->,
construct, destroy) remain the same as they rely on buffer being correctly
sized/aligned in class uninitialized.
---
Nitpick comments:
In `@src/core/promise.cxx`:
- Around line 218-219: Add a follow-up issue to track and resolve exception
cancel-safety referenced by the TODO in src/core/promise.cxx: note that a
stashed exception stored in uninitialized<std::exception_ptr> can leak if a
frame is cancelled before extract_exception() runs, describe the potential leak
scenario and desired outcomes (e.g., ensure stored exception is destroyed on
cancellation, or make storage RAII/cancel-safe), include reproduction steps or
unit-test ideas, and link the issue to the TODO comment so it can be picked up
later.
In `@src/utils/uninitialized.cxx`:
- Line 23: The operator-> implementation uses an unconventional std::bit_cast of
auto{buffer}; change it to a conventional pointer reinterpretation: return
std::launder(reinterpret_cast<T*>(buffer)); so locate operator-> (the auto
operator->() noexcept -> T * function) and replace the
std::bit_cast(auto{buffer}) expression with a reinterpret_cast to T* of the
internal buffer variable; keep std::launder and noexcept as-is and ensure buffer
refers to the stored std::byte array used by this uninitialized wrapper.
🪄 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: 34d86898-763b-41e6-8175-6994fb32936e
📒 Files selected for processing (5)
CMakeLists.txtsrc/core/frame.cxxsrc/core/promise.cxxsrc/utils/uninitialized.cxxsrc/utils/utils.cxx
ConorWilliams
force-pushed
the
v4-no-alloc-exception
branch
from
901158c to
2741d30
Compare
1 participant
Motivated by desire to keep every allocation customizable
Summary by CodeRabbit
New Features
uninitializedutility module for managing uninitialized object storage with type-safe construction and destruction.Refactor