[V4] No template-template#99
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 |
a96c20f to
9b05b25
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/poly_context.cxx (1)
32-53:⚠️ Potential issue | 🟡 MinorTypo and now-dead
post_error.
- Line 52: "re-introdcue" → "re-introduce".
- With
poly_context::postcommented out andpost(...)also removed fromderived_poly_context/mono_context(per the PR-wide refactor), nothing in the library now constructs or throwspost_error. Since it'sexport-ed, it remains part of the public ABI/API but is orphan/dead untilpost()is re-introduced. Consider either removing it (and restoring it alongsidepost()later) or adding a short comment explaining it's retained for the TODO.✏️ Proposed typo fix
- // TODO: re-introdcue post API if/when we support auto/scheduling contexts. + // TODO: re-introduce post API if/when we support auto/scheduling contexts. // virtual void post([[maybe_unused]] sched_handle<poly_context> handle) { LF_THROW(post_error{}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/poly_context.cxx` around lines 32 - 53, Fix the typo in the TODO comment by changing "re-introdcue" to "re-introduce", and address the dead exported type post_error: either remove the exported post_error type until poly_context::post (and related post implementations in derived_poly_context/mono_context) are restored, or keep post_error but add a short clarifying comment above it stating it is intentionally retained for future post() re-introduction and currently unused; ensure references to post_error, poly_context::post, derived_poly_context, and mono_context are mentioned in the comment so maintainers know why it remains exported.
🧹 Nitpick comments (1)
src/batteries/contexts.cxx (1)
49-49: Optional: propagatenoexceptonderived_poly_context::pushfor consistency withmono_context::push.
mono_context::push(line 71) isnoexcept(noexcept(m_container.push(handle))), but thefinaloverride here drops that. A virtual override is allowed to be more noexcept than the base (poly_context::pushhas nonoexcept), and since theDequepolicy'spushis what actually determines throwability, mirroringmono_contextavoids a false exception-edge forderived_poly_context.♻️ Suggested tweak
- constexpr void push(steal_handle<context_type> handle) final { m_container.push(handle); } + constexpr void push(steal_handle<context_type> handle) noexcept(noexcept(m_container.push(handle))) final { + m_container.push(handle); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/batteries/contexts.cxx` at line 49, The override derived_poly_context::push currently lacks a noexcept specifier and should mirror mono_context::push's conditional noexcept to avoid weakening exception guarantees; change derived_poly_context::push to be noexcept(noexcept(m_container.push(handle))) so its exception specification is determined by the underlying Deque's m_container.push, matching mono_context::push and preventing a spurious exception edge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/poly_context.cxx`:
- Around line 32-53: Fix the typo in the TODO comment by changing "re-introdcue"
to "re-introduce", and address the dead exported type post_error: either remove
the exported post_error type until poly_context::post (and related post
implementations in derived_poly_context/mono_context) are restored, or keep
post_error but add a short clarifying comment above it stating it is
intentionally retained for future post() re-introduction and currently unused;
ensure references to post_error, poly_context::post, derived_poly_context, and
mono_context are mentioned in the comment so maintainers know why it remains
exported.
---
Nitpick comments:
In `@src/batteries/contexts.cxx`:
- Line 49: The override derived_poly_context::push currently lacks a noexcept
specifier and should mirror mono_context::push's conditional noexcept to avoid
weakening exception guarantees; change derived_poly_context::push to be
noexcept(noexcept(m_container.push(handle))) so its exception specification is
determined by the underlying Deque's m_container.push, matching
mono_context::push and preventing a spurious exception edge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48df51f3-df4d-41c1-a914-5dbb4e50e63f
📒 Files selected for processing (7)
benchmark/src/libfork/fib.cppsrc/batteries/adaptors.cxxsrc/batteries/contexts.cxxsrc/core/concepts/context.cxxsrc/core/handles.cxxsrc/core/poly_context.cxxsrc/schedulers/busy.cxx
Summary by CodeRabbit