feat: migrate Wildcard to modern smart pointers #105
feat: migrate Wildcard to modern smart pointers #105luanluciano93 wants to merge 9 commits intoatlas-kit:devfrom
Conversation
ranisalt
force-pushed
the
wildcard-to-smart-pointers
branch
from
ec356c0 to
1dc25e3
Compare
📝 WalkthroughWalkthroughChanged Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
src/wildcardtree.h (1)
13-17: Consider declaring move assignment operator for consistency.The class explicitly defaults the move constructor but doesn't declare the move assignment operator. While the compiler-generated move assignment will work correctly with
unique_ptrmembers, explicitly declaring it would be more consistent with the Rule of Five pattern.🔎 Suggested addition
WildcardTreeNode(WildcardTreeNode&& other) = default; + WildcardTreeNode& operator=(WildcardTreeNode&&) = default; // non-copyable
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wildcardtree.cppsrc/wildcardtree.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: docker-image
🔇 Additional comments (7)
src/wildcardtree.h (2)
21-21: Good API design: returning reference instead of pointer.Returning a reference guarantees a valid node is always returned, eliminating null checks at call sites. This is a cleaner contract for
addChildsince it always succeeds.
29-29: Proper use ofstd::unique_ptrfor child node ownership.The migration to
std::unique_ptrcorrectly expresses exclusive ownership semantics and ensures automatic cleanup of child nodes.src/wildcardtree.cpp (5)
10-17: Correct accessor pattern for smart pointer storage.Both
getChildoverloads correctly use.get()to return a non-owning raw pointer, which is the appropriate pattern when exposing internalunique_ptr-managed objects without transferring ownership.Also applies to: 19-26
28-42: Well-implemented ownership transfer inaddChild.The implementation correctly:
- Checks for existing child and returns early (line 35)
- Captures the reference before moving the
unique_ptr(lines 37-39)- Returns the reference to the now-owned object (line 40)
This pattern avoids undefined behavior that would occur if you tried to dereference after
std::move.
44-54: Correctly adapted to newaddChildreturn type.Taking the address of the returned reference (
&cur->addChild(...)) is the correct way to continue traversal when the calling code uses pointers.
56-88:remove()correctly works with the new storage model.The removal logic properly:
- Traverses using
getChild()which returns raw pointers- Erases from the map, which automatically destroys the
unique_ptrand its managed node
110-113: Correct traversal update for smart pointer storage.Using
it->second.get()correctly obtains the raw pointer for traversal without affecting ownership.
|
I don't think this one helps, it's creating a second indirection to access the nodes and it was already not using pointers. It would maybe be a better approach for these methods to return |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/wildcardtree.cpp:
- Around line 10-26: The current WildcardTreeNode::getChild overloads return
std::optional<std::reference_wrapper<...>> which forces verbose usage; change
both overloads to return raw pointers (WildcardTreeNode* for the non-const and
const WildcardTreeNode* for the const overload), have them look up `children`
and return the found node pointer or nullptr when not found, and update call
sites to check for nullptr (or add a simple hasChild(char) that uses
children.find to preserve existing call patterns). Ensure both
WildcardTreeNode::getChild implementations and any call sites using ->get() or
.value().get() are updated accordingly.
- Line 30: There is a stray empty statement (a lone ';') in src/wildcardtree.cpp
left from refactoring; remove that semicolon so there is no standalone empty
statement in the file (ensure surrounding code compiles cleanly after removal).
🧹 Nitpick comments (1)
src/wildcardtree.cpp (1)
35-35: Consider more explicit dereferencing.While
return *node;works due to implicit conversion fromstd::reference_wrapper<T>toT&, it's not immediately obvious. Consider usingreturn node->get();for clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wildcardtree.cppsrc/wildcardtree.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wildcardtree.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/wildcardtree.cpp (1)
src/wildcardtree.h (1)
WildcardTreeNode(9-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: windows
- GitHub Check: ubuntu
- GitHub Check: docker-image
- GitHub Check: test
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/wildcardtree.cpp (3)
42-52: Implementation is correct.The use of
&cur->addChild(...)to convert the returned reference to a pointer is valid. However, this pattern highlights the mixed pointer/reference style introduced by the new API.
54-86: Implementation is correct.The usage of
node->get()to access the wrapped reference is correct. While verbose, it's consistent with the new API design.
88-113: Implementation is correct.The method correctly uses the new API, including proper handling of
std::optionaland structured binding for map iteration.
| std::optional<std::reference_wrapper<WildcardTreeNode>> WildcardTreeNode::getChild(char ch) | ||
| { | ||
| auto it = children.find(ch); | ||
| if (it == children.end()) { | ||
| return nullptr; | ||
| return std::nullopt; | ||
| } | ||
| return &it->second; | ||
| return it->second; | ||
| } | ||
|
|
||
| const WildcardTreeNode* WildcardTreeNode::getChild(char ch) const | ||
| std::optional<std::reference_wrapper<const WildcardTreeNode>> WildcardTreeNode::getChild(char ch) const | ||
| { | ||
| auto it = children.find(ch); | ||
| if (it == children.end()) { | ||
| return nullptr; | ||
| return std::nullopt; | ||
| } | ||
| return &it->second; | ||
| return it->second; | ||
| } |
There was a problem hiding this comment.
Consider the added complexity of std::optional<std::reference_wrapper>.
While the implementation is correct, the reviewer ranisalt raised a valid concern: this API introduces additional indirection and complexity compared to returning a raw pointer. The pattern std::optional<std::reference_wrapper<T>> requires verbose access patterns (->get() or .value().get()) at every call site, as seen throughout the code.
Consider alternatives that maintain safety while reducing complexity:
- Return
WildcardTreeNode*(nullptr on not found) - simpler and direct - Restructure the API to avoid optional returns (e.g., separate
hasChild+getChildmethods, or use iterators) - Wait for C++23's
std::optional<T&>support (if applicable)
Based on the PR comment from ranisalt.
🤖 Prompt for AI Agents
In @src/wildcardtree.cpp around lines 10 - 26, The current
WildcardTreeNode::getChild overloads return
std::optional<std::reference_wrapper<...>> which forces verbose usage; change
both overloads to return raw pointers (WildcardTreeNode* for the non-const and
const WildcardTreeNode* for the const overload), have them look up `children`
and return the found node pointer or nullptr when not found, and update call
sites to check for nullptr (or add a simple hasChild(char) that uses
children.find to preserve existing call patterns). Ensure both
WildcardTreeNode::getChild implementations and any call sites using ->get() or
.value().get() are updated accordingly.
| if (child) { | ||
| if (breakpoint && !child->breakpoint) { | ||
| child->breakpoint = true; | ||
| ; |
There was a problem hiding this comment.
Remove the empty statement.
Line 30 contains an empty statement ; that appears to be leftover from refactoring.
🔎 Proposed fix
WildcardTreeNode& WildcardTreeNode::addChild(char ch, bool breakpoint)
{
- ;
if (const auto& node = getChild(ch)) {🤖 Prompt for AI Agents
In @src/wildcardtree.cpp at line 30, There is a stray empty statement (a lone
';') in src/wildcardtree.cpp left from refactoring; remove that semicolon so
there is no standalone empty statement in the file (ensure surrounding code
compiles cleanly after removal).
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 2 medium |
🟢 Metrics 0 complexity
Metric Results Complexity 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…ection - remove() was not advancing cur through the loop, so children were looked up on the root and the breakpoint was cleared on the wrong node. - Reverted std::map<char, std::unique_ptr<T>> to std::map<char, T>; map storage is already stable and the extra heap allocation per node was unnecessary. - Added explicit <functional> and <optional> includes; replaced piecewise emplace with try_emplace so <tuple> is no longer needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Pull Request Prelude
Changes Proposed
Issues addressed: #40
How to test:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.