Repository navigation
[STF] errsink: a named dynamic is viewed, not cloned; an unhooked success passes unboxed - #12325
Conversation
The left operand of a sink's operators, `__sink_at<Self>`, held the sink by value. For `dynamic` that copy clones the erased model, so every guarded call on a named erased sink allocated. The capture now holds `__sink_storage_t<Self>`, which is the sink itself for every concrete policy and `__dynamic_view` for `dynamic`: a non-const lvalue is viewed, an rvalue is moved in (a pointer move), and a const lvalue is still copied, since its hooks would otherwise run on a const object. The friend operators name their first parameter through `__sink_at_for<Self, X>` so that the capture type depends on `X`: defined inside the class, they would otherwise instantiate `__sink_at<dynamic>` before its storage type can be complete. Ports the first half of NVIDIA#12000 (Cédric Augonnet) onto the sink-is-its-own- carrier design of NVIDIA#12107. Co-Authored-By: Cédric Augonnet <caugonnet@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e unboxed `sink_base` records at erasure time whether the wrapped policy has a success hook. When it has none, `dynamic::on_success` returns the callable's value as it is instead of boxing it in `std::any`, calling the model, and unboxing it. Hand-written models keep the conservative default and still go through the box. Ports the second half of NVIDIA#12000 (Cédric Augonnet) onto NVIDIA#12107. Co-Authored-By: Cédric Augonnet <caugonnet@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/ok to test e9cd371 |
📝 Summary
Priority: Change: Bug fix Merge Risk:
 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: NVIDIA/cccl
- Branch: main
- File: .coderabbit.yaml
- Commit: 06b483c
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
ef563774-1252-4d64-a1f4-c7e11ce1a563
-
cudax/include/cuda/experimental/__stf/utility/exception_policy.cuh
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
This comment has been minimized.
This comment has been minimized.
…ent the viewed dynamic The test's local `counting` class was not a policy under GCC: its raw hook took `std::function&`, which the capability probe's archetype cannot bind to, so normalization made it a `subst_t` and the success hook never ran. The policy is now at namespace scope with the templated `on_thrown` hook, and the test asserts the capability it relies on. The `sink` overview says a named sink is copied into each expression; a named non-const `dynamic` is viewed instead, so the paragraph says so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/ok to test 432fd94 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
exception_policy.cuh:5021
cudax/include/cuda/experimental/__stf/utility/exception_policy.cuh:5021
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winimportant: Keep dynamic carriers copyable.
auto c = d << [] {}; c ->* [] {};no longer compiles becauseoperator->*takes the carrier by value and this constructor is deleted. The documented carrier API supports->*, and the previous dynamic carrier was copyable. Implement copy semantics for the view, including its owned-storage case.
exception_policy.cuh:5007
cudax/include/cuda/experimental/__stf/utility/exception_policy.cuh:5007
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftimportant: Returned carriers can hold a dangling pointer to a named
dynamic.
__dynamic_view(dynamic& __d)stores&__d, and its move constructor keeps that pointer for lvalue views. A carrier built fromd << fis returned by value through the sink operators. If it leaves the scope that ownsd, for exampleauto make() { dynamic d{...}; return d << f; }, the carrier points to a destroyed object. A later<<or->*then reads through that pointer.Before this change, the carrier held a cloned
dynamic, so this return was valid. The sink overview says a named non-constdynamicis viewed and that a carrier can outlive a statement. It does not say that the carrier must not outlive the nameddynamic. The current code has no guard for this case.Decide the lifetime contract before changing the code. If a carrier from a named lvalue may escape its scope, the view must not be a raw pointer. Giving the carrier shared ownership of the model changes what copying a
dynamicdoes, so it is not a local fix. If the view is meant to stay statement-scoped, state that restriction in the sink overview and in thedynamicoperator documentation.
-
-
- Source mode: Auto
- Repository: NVIDIA/cccl
- Branch: main
- File: .coderabbit.yaml
- Commit: 06b483c
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
e1e0737f-6f98-4cb6-8a03-8fdab4c77319
-
cudax/include/cuda/experimental/__stf/utility/exception_policy.cuh
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
🥳 CI Workflow Results🟩 Finished in 1h 16m: Pass: 100%/73 | Total: 19h 02m | Max: 1h 16m | Hits: 79%/58411See results here. |
Ports the two shell fixes of #12000 (@caugonnet) onto the sink-is-its-own-carrier design that landed in #12107.
A named
dynamicis viewed by<<and->*, not cloned. The left operand of a sink's operators,__sink_at<Self>, held the sink by value; fordynamicthat copy clones the erased model, so every guarded call on a named erased sink allocated. The capture now holds__sink_storage_t<Self>: the sink itself for concrete policies,__dynamic_viewfordynamic. A non-const lvalue is viewed, an rvalue is moved in (a pointer move), and a const lvalue is still copied, since its hooks would otherwise run on a const object. The friend operators name their first parameter through__sink_at_for<Self, X>so that, defined insidedynamic, they do not instantiate the capture before its storage type is complete.A
dynamicover a policy without a success hook passes the value unboxed.sink_baserecords at erasure time whether the wrapped policy has a success hook. When it has none,dynamic::on_successreturns the callable's value as it is, skipping thestd::anybox, virtual call, and unbox. Hand-written models keep the conservative default.Tests: a counting model shows zero clones for lvalue and rvalue use and one for a const lvalue; a copy-counting result type shows zero copies through
dynamic(noop)and a hooked policy still sees the value. Host run of the header's unit tests, the 16 static-error tests, and a no-exceptions syntax pass are clean.These are the two fixes the paper's dynamic-column numbers were measured on (#12000 head). Once this lands, #12000 can be closed.
🤖 Generated with Claude Code