Open
Conversation
Replace manual memory management with RAII using std::unique_ptr for: - nRadixNode members (value_, left_, right_) - NRadixTree root_ member This eliminates manual delete calls, simplifies copy/assignment operators, and prevents potential memory leaks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactored Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@collector/lib/NRadix.h`:
- Around line 56-63: The copy-assignment operator for nRadixNode causes infinite
recursion because std::swap falls back to copy assignment when no move
operations exist; declare and default the move constructor and move-assignment
operator for nRadixNode (e.g., nRadixNode(nRadixNode&&) noexcept = default; and
nRadixNode& operator=(nRadixNode&&) noexcept = default;) so std::swap will use
move semantics, or alternatively implement a noexcept member swap and call that
from operator=; update the class declaration to include these move operations
(and mark them noexcept) so the copy-and-swap implementation of operator= stops
recursing.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8a027223-4a87-4a0a-be65-620317f28f34
📒 Files selected for processing (2)
collector/lib/NRadix.cppcollector/lib/NRadix.h
Comment on lines
56
to
63
| nRadixNode& operator=(const nRadixNode& other) { | ||
| if (this == &other) { | ||
| return *this; | ||
| } | ||
| auto* new_node = new nRadixNode(other); | ||
| auto new_node = std::make_unique<nRadixNode>(other); | ||
| std::swap(*new_node, *this); | ||
| delete new_node; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Infinite recursion: std::swap falls back to copy assignment without move operations.
Since nRadixNode has a user-declared copy constructor and copy assignment, the move constructor and move assignment operator are not implicitly generated. When std::swap(*new_node, *this) executes, it attempts to use move semantics but falls back to copy operations. The internal a = std::move(b) call invokes this same operator=, causing infinite recursion and stack overflow.
Add move operations to enable proper swap semantics:
🐛 Proposed fix: add defaulted move operations
nRadixNode(const nRadixNode& other) : value_(nullptr), left_(nullptr), right_(nullptr) {
if (other.value_) {
value_ = std::make_unique<IPNet>(*other.value_);
}
if (other.left_) {
left_ = std::make_unique<nRadixNode>(*other.left_);
}
if (other.right_) {
right_ = std::make_unique<nRadixNode>(*other.right_);
}
}
+ nRadixNode(nRadixNode&&) = default;
+ nRadixNode& operator=(nRadixNode&&) = default;
+
nRadixNode& operator=(const nRadixNode& other) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@collector/lib/NRadix.h` around lines 56 - 63, The copy-assignment operator
for nRadixNode causes infinite recursion because std::swap falls back to copy
assignment when no move operations exist; declare and default the move
constructor and move-assignment operator for nRadixNode (e.g.,
nRadixNode(nRadixNode&&) noexcept = default; and nRadixNode&
operator=(nRadixNode&&) noexcept = default;) so std::swap will use move
semantics, or alternatively implement a noexcept member swap and call that from
operator=; update the class declaration to include these move operations (and
mark them noexcept) so the copy-and-swap implementation of operator= stops
recursing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3160 +/- ##
==========================================
+ Coverage 27.38% 27.42% +0.03%
==========================================
Files 95 95
Lines 5427 5423 -4
Branches 2548 2540 -8
==========================================
+ Hits 1486 1487 +1
Misses 3214 3214
+ Partials 727 722 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/retest collector-on-push |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.