Update core snapshot to netft-cpp 0.3.3 - #6
Conversation
📝 WalkthroughWalkthroughThe project pins netft-cpp 0.3.3, removes CLI build configuration, and adds deferred ChangesNative core upgrade
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SampleCallback
participant Client
participant ClientImpl
participant DeferredDestroyer
SampleCallback->>Client: destroy Client
Client->>ClientImpl: called_from_worker_thread()
Client->>ClientImpl: stop()
Client->>DeferredDestroyer: queue Impl
DeferredDestroyer->>ClientImpl: destroy Impl asynchronously
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/src/client.cpp (1)
14-57: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNote the serialization behavior of the shared
DeferredDestroyer.
DeferredDestroyer<Impl>is a single, process-lifetime static shared across allClientinstances.run()processes one queuedImplat a time, and eachstop()call can block onjoin()until that particular worker thread exits. If severalClientinstances are deferred-destroyed from their worker threads around the same time, their destruction is fully serialized on this one background thread.This is unlikely to matter for a small number of sensor clients, but consider documenting the constraint, or using one worker per pending item if many concurrent
Clientinstances are expected.🤖 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 `@core/src/client.cpp` around lines 14 - 57, Document the serialization constraint of the shared DeferredDestroyer<Impl>: its single worker processes pending stop() calls sequentially, so deferred destruction across concurrent Client instances is serialized. If the expected client count requires concurrent cleanup, revise DeferredDestroyer to dispatch each pending item independently while preserving safe shutdown and completion behavior.
🤖 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.
Inline comments:
In `@core/CMakeLists.txt`:
- Around line 3-9: Remove all stale NETFT_BUILD_CLI references from the project
CMake configuration and CI workflow, including its option/setting and any
conditional uses, while leaving unrelated build options and workflow behavior
unchanged.
In `@core/src/detail/client_impl.cpp`:
- Around line 289-293: Update Client::Impl::called_from_worker_thread() to use
the active-worker predicate based on worker_exited_ and active_worker_id_,
matching the logic in start() and stop(), rather than worker_.joinable() and
worker_.get_id(). Add a regression test covering the interval after stop() moves
worker_ but before join completes, ensuring callback-triggered Client
destruction does not destroy Impl while handle_record() is still running.
---
Nitpick comments:
In `@core/src/client.cpp`:
- Around line 14-57: Document the serialization constraint of the shared
DeferredDestroyer<Impl>: its single worker processes pending stop() calls
sequentially, so deferred destruction across concurrent Client instances is
serialized. If the expected client count requires concurrent cleanup, revise
DeferredDestroyer to dispatch each pending item independently while preserving
safe shutdown and completion behavior.
🪄 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 Plus
Run ID: 7976aa6d-04cb-4e0f-8986-76aa9f38c2c8
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdcore/CMakeLists.txtcore/SNAPSHOT.sha256core/UPSTREAMcore/include/netft/client.hppcore/src/client.cppcore/src/detail/client_impl.cppcore/src/detail/client_impl.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@core/UPSTREAM`:
- Around line 2-3: Align the release version across core/UPSTREAM,
core/CMakeLists.txt, and the PR summary so they all name the same netft-cpp
version; either update the summary to 0.3.3 or change the pinned and declared
version consistently to 0.3.2.
🪄 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 Plus
Run ID: c2bf37a5-8506-4f55-8109-a3421b39972e
📒 Files selected for processing (9)
.github/workflows/ci.ymlCHANGELOG.mdCMakeLists.txtREADME.mdcore/CMakeLists.txtcore/SNAPSHOT.sha256core/UPSTREAMcore/src/client.cppcore/src/detail/client_impl.hpp
💤 Files with no reviewable changes (2)
- .github/workflows/ci.yml
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- CHANGELOG.md
- core/SNAPSHOT.sha256
- core/CMakeLists.txt
- core/src/client.cpp
Summary
Verification