Cleanup wto: cpp+hpp, remove recursive version #699
Conversation
Signed-off-by: Elazar Gershuni <[email protected]>
WalkthroughThe changes involve significant modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range comments (1)
src/crab/wto.cpp (1)
Line range hint
55-64: Potential logical error instart_visit: Overwritingmin_dfninstead of minimizingIn the loop,
min_dfnis assigned directly fromdata.head_dfnordata.dfn, which overwrites its previous value. This approach only considers the last successor'sdfn, rather than finding the minimumdfnamong all successors. To correctly compute the minimumdfn, usestd::minto updatemin_dfn.Apply the following diff to fix the issue:
for (const label_t& succ : _cfg.next_nodes(vertex)) { const wto_vertex_data_t& data = _vertex_data[succ]; if (data.head_dfn != 0 && data.dfn != INT_MAX) { - min_dfn = data.head_dfn; + min_dfn = std::min(min_dfn, data.head_dfn); } else { - min_dfn = data.dfn; + min_dfn = std::min(min_dfn, data.dfn); } if (min_dfn <= head_dfn) { head_dfn = min_dfn; loop = true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- src/crab/wto.cpp (8 hunks)
- src/crab/wto.hpp (3 hunks)
- src/crab/wto_cycle.hpp (0 hunks)
- src/crab/wto_nesting.hpp (0 hunks)
💤 Files with no reviewable changes (2)
- src/crab/wto_cycle.hpp
- src/crab/wto_nesting.hpp
🧰 Additional context used
🪛 cppcheck
src/crab/wto.cpp
[style] 229-229: The function 'nesting' is never used.
(unusedFunction)
🔇 Additional comments (5)
src/crab/wto.hpp (1)
51-51: Confirm Availability of 'std::ranges::reverse_view'The code uses
std::ranges::reverse_viewin theoperator<<overload forwto_nesting_t. This requires C++20 support. Please verify that the project is configured to use C++20 standard or replace with an alternative approach compatible with the project's C++ standard.src/crab/wto.cpp (4)
8-10: LGTM!The added comments enhance clarity by explaining how
_visit_stackavoids stack overflows compared to the recursive implementation.
11-27: Implementation ofoperator>is correct and efficientThe
operator>function forwto_nesting_tcorrectly compares nesting levels to determine if one nesting is a superset of another. The logic efficiently handles the comparisons starting from the outermost nesting.
30-30: Passingcontaining_cycleas aconst std::weak_ptrimproves performanceChanging the
containing_cycleparameter to aconst std::weak_ptr<wto_cycle_t>&avoids unnecessary copies and potential overhead, improving performance.Also applies to: 55-55, 87-87, 112-112
39-39: Use ofemplaceenhances efficiencyReplacing
pushwithemplacein_visit_stackoperations constructs the objects in place, eliminating unnecessary copies and improving performance.Also applies to: 43-43
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (1)
src/crab/wto.cpp (1)
Line range hint
124-134: Potential stack underflow instart_visitfunctionIn the
whileloop starting at line 126, there's a risk of calling_stack.top()and_stack.pop()on an empty stack ifelement == vertexon the first iteration or if thevertexis not found in the stack. This could lead to undefined behavior or a runtime exception.Consider adding a check to ensure the stack is not empty before accessing it:
while (element != vertex) { _vertex_data[element].dfn = 0; _vertex_data[element].head_dfn = 0; + if (_stack.empty()) { + // Handle the error or break out of the loop + break; + } element = _stack.top(); _stack.pop(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- src/crab/fwd_analyzer.cpp (7 hunks)
- src/crab/thresholds.cpp (2 hunks)
- src/crab/thresholds.hpp (1 hunks)
- src/crab/wto.cpp (6 hunks)
- src/crab/wto.hpp (2 hunks)
🧰 Additional context used
🪛 cppcheck
src/crab/wto.cpp
[style] 286-286: The function 'nesting' is never used.
(unusedFunction)
🔇 Additional comments (10)
src/crab/thresholds.hpp (1)
68-68: Improved const-correctness in operator() methodThis change enhances the method's const-correctness by modifying the parameter type from
std::shared_ptr<wto_cycle_t>&toconst std::shared_ptr<wto_cycle_t>&. This is a positive improvement for several reasons:
- It clearly communicates that the method doesn't modify the shared_ptr itself.
- It allows the method to be called with both const and non-const shared_ptr objects, increasing its flexibility.
- It adheres to the C++ best practice of accepting parameters by const reference when modification isn't needed.
- The change is backwards compatible and doesn't affect the ability to modify the pointed-to object if necessary.
This modification contributes to a more robust and maintainable codebase by enforcing const-correctness.
src/crab/thresholds.cpp (3)
71-71: Approve: Improved parameter type for better const-correctnessThe change from
std::shared_ptr<wto_cycle_t>&toconst std::shared_ptr<wto_cycle_t>&is a good practice. It clearly indicates that the method won't modify the shared pointer itself, enhancing type safety and making the code's intent more explicit.
Line range hint
71-88: Summary: Changes align with PR objectivesThe modifications in this file, particularly in the
wto_thresholds_t::operator()method, are consistent with the PR's objectives of cleaning up the WTO (Well-Founded Ordering) algorithm implementation. The changes improve const-correctness and suggest a restructuring of how components are handled, which aligns with the goal of removing the recursive version and enhancing code organization.
88-88: Verify the impact of removing dereferencing in std::visitThe change from
std::visit(*this, *component)tostd::visit(*this, component)suggests a modification in how components are stored or accessed. This could be related to changes in thewto_cycle_tclass or its container type.Please run the following script to check for any related changes in the
wto_cycle_tclass and its usage:✅ Verification successful
Impact of Removing Dereferencing in
std::visitVerifiedThe change from
std::visit(*this, *component)tostd::visit(*this, component)insrc/crab/thresholds.cpp:88is isolated and does not affect other parts of the codebase. No related changes were found in thewto_cycle_tclass or its usage elsewhere.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for changes in wto_cycle_t class and its usage # Search for wto_cycle_t definition echo "wto_cycle_t definition:" rg --type cpp "class\s+wto_cycle_t" -A 10 # Search for wto_cycle_t usage in containers echo "\nwto_cycle_t usage in containers:" rg --type cpp "(?:vector|list|set)<.*wto_cycle_t.*>" # Search for other occurrences of std::visit with wto_cycle_t echo "\nOther occurrences of std::visit with wto_cycle_t:" rg --type cpp "std::visit.*wto_cycle_t"Length of output: 1303
Script:
#!/bin/bash # Description: Further check for std::visit usages involving wto_cycle_t through type aliases and indirect references # Search for std::visit usages without specifying wto_cycle_t directly echo "std::visit usages without direct wto_cycle_t reference:" rg --type cpp "std::visit\s*\(.*\wto_cycle_t.*\)" # Search for type aliases related to wto_cycle_t and their usage with std::visit echo "\nType aliases related to wto_cycle_t used with std::visit:" rg --type cpp "using\s+\w+\s+=\s+.*wto_cycle_t.*" | while read -r alias; do alias_name=$(echo "$alias" | awk '{print $2}') rg --type cpp "std::visit\s*\(.*$alias_name.*\)" done # Search for std::visit in files that include wto_cycle_t definition echo "\nstd::visit usages in files including wto_cycle_t definition:" rg --type cpp -l "wto_cycle_t" | xargs rg --type cpp "std::visit"Length of output: 1254
src/crab/fwd_analyzer.cpp (6)
30-30: Improved const-correctness inmember_component_visitor::operator()The change from
std::shared_ptr<wto_cycle_t>&toconst std::shared_ptr<wto_cycle_t>&improves const-correctness. This is a good practice in C++ as it clearly communicates that the method doesn't modify the shared_ptr itself.
38-38: Consistent update instd::visitcallThe change from
std::visit(*this, *component)tostd::visit(*this, component)is consistent with the updated method signature. Sincecomponentis now a reference to ashared_ptr, it doesn't need to be dereferenced before passing tostd::visit. This change maintains the correct behavior while simplifying the code.
117-117: Consistent const-correctness improvement ininterleaved_fwd_fixpoint_iterator_t::operator()Similar to the change in
member_component_visitor, this update fromstd::shared_ptr<wto_cycle_t>&toconst std::shared_ptr<wto_cycle_t>&improves const-correctness. This consistent application of const-correctness across the codebase is a good practice that enhances code quality and maintainability.
128-129: Improved memory management withstd::shared_ptrThe changes from raw pointers to
std::shared_ptrforwto_cycle_tare a significant improvement:
- Line 128: Using
std::holds_alternative<std::shared_ptr<wto_cycle_t>>instead ofstd::holds_alternative<wto_cycle_t*>.- Line 129: Using
std::get<std::shared_ptr<wto_cycle_t>>instead ofstd::get<wto_cycle_t*>.These changes align with modern C++ best practices by leveraging smart pointers. This can help prevent memory leaks, improve resource management, and make ownership semantics clearer. Good job on this refactoring!
193-194: Refactoring to usestd::variantandstd::visitThe changes in the
interleaved_fwd_fixpoint_iterator_t::operator()method (lines 193-194 and 213-214) reflect a shift towards usingstd::variantandstd::visit:
- The condition
!std::holds_alternative<label_t>(component) || (std::get<label_t>(component) != head)checks if the component is not alabel_tor if it's alabel_tbut not equal to the head.- The use of
std::visit(*this, component)is consistent with the changes seen earlier in the file.These changes likely replace a custom polymorphic approach with the more type-safe and efficient
std::variantandstd::visit. This refactoring can lead to more robust and easier to maintain code, as it leverages the type system to handle different component types safely.Also applies to: 213-214
Line range hint
1-231: Overall assessment of changesThe changes in this file represent a significant improvement in code quality and adherence to modern C++ best practices:
- Improved const-correctness throughout the file.
- Shift from raw pointers to
std::shared_ptrfor better memory management.- Refactoring to use
std::variantandstd::visitfor type-safe handling of heterogeneous collections.These changes should lead to more robust, maintainable, and efficient code. They appear to be part of a larger effort to modernize the codebase, which is commendable. The consistent application of these improvements across different parts of the file is particularly noteworthy.
Great job on this refactoring effort!
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/crab/wto.cpp (6 hunks)
- src/crab/wto.hpp (2 hunks)
🧰 Additional context used
🪛 cppcheck
src/crab/wto.cpp
[style] 286-286: The function 'nesting' is never used.
(unusedFunction)
🔇 Additional comments (12)
src/crab/wto.hpp (3)
36-47: Well-Structured Implementation ofwto_nesting_tClassThe
wto_nesting_tclass is effectively designed to manage the collection of heads of nested components. The use of reverse order storage for optimizing insertion performance is a thoughtful approach. The constructor correctly utilizes move semantics to efficiently initialize_heads.
67-93: Robust Handling inwto_cycle_tClassThe
wto_cycle_tclass appropriately encapsulates cycles within the weak topological ordering. Thehead()method includes necessary checks for empty components and safely accesses the variant, preventing potential runtime errors. The use of[[nodiscard]]attributes for the iterators enhances code correctness by encouraging the handling of return values.
114-127: Appropriate Usage of Constructors and Iterators inwto_tClassThe
wto_tclass constructor is correctly marked asexplicit, preventing unintended implicit conversions. The iterator methodsbegin()andend()are properly implemented, providing const-correct reverse iterators over the components. The inclusion of thenesting()method is well-placed for accessing the nesting information of a label.src/crab/wto.cpp (9)
13-29: Implementation ofoperator>is correct and efficientThe
operator>overload forwto_nesting_tcorrectly compares nesting structures by size and content, iterating from the outermost elements inward. The logic is sound and efficiently implemented.
68-73: Consistent use ofconstreferences for parametersThe functions
push_successors,start_visit, andcontinue_visitnow correctly useconst std::weak_ptr<wto_cycle_t>&for thecontaining_cycleparameter, improving performance by avoiding unnecessary copies.
Line range hint
121-140: Efficient creation of shared pointersThe use of
std::make_sharedto create new cycles withcontaining_cycleis efficient and avoids unnecessary copies. The constructor parameters are correctly forwarded.
227-232: Usage of C++20<ranges>libraryThe use of
std::ranges::reverse_viewprovides a clean and efficient way to iterate in reverse. Ensure that the project's build configuration supports C++20 standards to prevent any compilation issues.
276-282: Correct implementation ofcollect_headsfunctionThe
collect_headsfunction effectively gathers all the heads of nested components containing a given label, handling optional values appropriately without risk of infinite loops.
286-293: Acknowledging the unusednestingfunctionAs noted by static analysis tools, the
nestingfunction is currently unused. Since this was previously flagged, no additional action is required unless new developments necessitate its usage.🧰 Tools
🪛 cppcheck
[style] 286-286: The function 'nesting' is never used.
(unusedFunction)
242-245: Overload ofoperator<<forwto_tThe
operator<<overload forwto_tenhances debug capabilities by allowing easy printing ofwto_tobjects. The implementation is appropriate and adheres to best practices.
210-225: Correct use of visitor pattern inprint_visitorThe
print_visitorclass effectively implements the visitor pattern to handle different component types. The overloads are correctly defined, and shared pointers are safely dereferenced after null checks.
253-272: Proper handling of optional values inheadmethodThe
headmethod correctly navigates the nested cycles to find the encompassing head label, handlingstd::optionaland weak pointers safely to prevent dereferencing null or expired pointers.
A snippet from #673: remove recursive version of the WTO algorithm, and split to header and implementation.
wto_tclass implementation and removed unnecessary conditional compilations.