Add target() to executor_ref and execution_context#154
Add target() to executor_ref and execution_context#154sgerbino merged 1 commit intocppalliance:developfrom
Conversation
Replace executor_ref::type_id() with a type-safe target<T>() accessor modeled after std::any_cast. Add the same target<T>() API to execution_context for downcasting to a derived context type. Add corresponding unit tests for both classes.
📝 WalkthroughWalkthroughAdded per-instance type identification and downcasting capabilities to execution contexts and executor references. Introduced Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![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
🤖 Fix all issues with AI agents
In `@include/boost/capy/ex/executor_ref.hpp`:
- Around line 273-280: The non-const target() in executor_ref exposes a
const_cast that can yield UB when executor_ref was constructed from an Ex
const&, so remove the mutable overload to ensure executor_ref only provides a
const view: delete the template< typename Executor> Executor* target() that
returns const_cast of ex_, leaving (or adding) only a const overload returning
Executor const*; update any references/docs/comments for executor_ref, target(),
vt_->type_id and ex_ to reflect that callers must not obtain a mutable pointer
from executor_ref (or alternatively add a prominent `@warning` in the header if
you prefer the documented-but-unsafe behavior).
🧹 Nitpick comments (6)
include/boost/capy/ex/executor_ref.hpp (2)
264-270: Marktarget()asnoexcept.All operations inside (
*vt_->type_id, comparison,static_cast) are non-throwing, and the analogousstd::any_caston pointers is alsonoexcept. Both overloads should be markednoexceptfor consistency with the model and to allow callers to rely on the no-throw guarantee.Proposed fix
template< typename Executor > - const Executor* target() const + const Executor* target() const noexcept {And similarly for the non-const overload:
template< typename Executor> - Executor* target() + Executor* target() noexcept {
251-263: Documentation is missing@par Exampleand@seesections.Per the coding guidelines for function javadocs in headers, include
@par Examplewith a@codeblock showing typical usage (e.g., checking and downcasting), and@seeat the end cross-referencing related types likeexecution_context::target,any_executor, orstd::any_cast.As per coding guidelines, "Include
@parExample in function javadocs with one or more@codeblocks" and "Include@seein function javadocs (always last) for related functions, types, and concepts".include/boost/capy/ex/execution_context.hpp (4)
437-452: Marktarget()overloads asnoexcept.Same as
executor_ref::target()— all operations (null check,type_infocomparison,static_cast) are non-throwing. Marking thesenoexceptis consistent withstd::any_caston pointers.Proposed fix
template< typename ExecutionContext > - const ExecutionContext* target() const + const ExecutionContext* target() const noexcept {template< typename ExecutionContext > - ExecutionContext* target() + ExecutionContext* target() noexcept {
99-101: Protected constructor lacks documentation.This constructor is part of the protected API that derived classes must use to enable
target(). A brief docstring explaining its purpose, theDerived*parameter (used only for type deduction), and the postcondition (target<Derived>()will succeed) would help derived-class authors.As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces."
424-436: Documentation is missing@par Exampleand@seesections.Same as
executor_ref::target()— the coding guidelines require examples and cross-references for function javadocs in headers. A short example showing a derived class using theDerived*constructor and then callingtarget()would be helpful.As per coding guidelines, "Include
@parExample in function javadocs with one or more@codeblocks" and "Include@seein function javadocs (always last) for related functions, types, and concepts".
88-88: Consider placingti_near the other data members at the bottom of the class.
ti_is declared at the very top of the class (line 88) while all other data members (mutex_,owned_,frame_alloc_,head_,shutdown_) are grouped together in theprivate:section at lines 539-546. Placingti_with them improves layout clarity and keeps the data member declarations cohesive.
| template< typename Executor> | ||
| Executor* target() | ||
| { | ||
| return *vt_->type_id; | ||
| if ( *vt_->type_id == detail::type_id< Executor >() ) | ||
| return const_cast< Executor* >( | ||
| static_cast< Executor const* >( ex_ )); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Non-const target() casts away constness of a const-captured executor.
executor_ref captures the executor via Ex const& (line 153), storing it as void const*. The non-const target() then uses const_cast to return a mutable Executor*. If a caller mutates through this pointer, that's UB whenever the original executor was declared const.
This is consistent with the existing vtable lambdas (e.g., context(), on_work_started()) which also const_cast internally, so this is a pre-existing design choice. However, target() exposes the cast directly to user code, which is more hazardous. Consider either:
- Adding a
@noteor@warningin the documentation stating that modification through the returned pointer is undefined behavior if the original executor wasconst, or - Removing the non-const overload entirely, since
executor_reffundamentally holds a const view.
🤖 Prompt for AI Agents
In `@include/boost/capy/ex/executor_ref.hpp` around lines 273 - 280, The non-const
target() in executor_ref exposes a const_cast that can yield UB when
executor_ref was constructed from an Ex const&, so remove the mutable overload
to ensure executor_ref only provides a const view: delete the template< typename
Executor> Executor* target() that returns const_cast of ex_, leaving (or adding)
only a const overload returning Executor const*; update any
references/docs/comments for executor_ref, target(), vt_->type_id and ex_ to
reflect that callers must not obtain a mutable pointer from executor_ref (or
alternatively add a prominent `@warning` in the header if you prefer the
documented-but-unsafe behavior).
|
An automated preview of the documentation is available at https://154.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-12 03:31:42 UTC |
|
GCOVR code coverage report https://154.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-12 03:39:09 UTC |
2 participants
Replace executor_ref::type_id() with a type-safe target() accessor modeled after std::any_cast. Add the same target() API to execution_context for downcasting to a derived context type. Add corresponding unit tests for both classes.
Summary by CodeRabbit