Const all the things #778
Const all the things #778
Conversation
also some modernization Signed-off-by: Elazar Gershuni <[email protected]>
WalkthroughThe pull request introduces a series of modifications across multiple source files, primarily focusing on enhancing code readability, maintainability, and type safety. Key changes include updating variable declarations to use Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (9)src/main/check.cpp (4)
The change from
Using
Using
The addition of Also applies to: 268-269 src/crab/cfg.hpp (5)
The change to
The change improves const-correctness by preventing accidental modifications to the loop variable.
Using the iterator range constructor for the set is more efficient than inserting elements one by one.
Good consistent change from
Also applies to: 575-575, 579-579, 594-594, 599-599
The change improves const-correctness by preventing accidental modifications to the loop variables. Also applies to: 593-594 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: 21
🧹 Outside diff range comments (6)
src/crab/thresholds.cpp (1)
Line range hint
39-47: Modernize: Use range-based for loop for better readability.The current iterator-based loop can be simplified.
Consider this more modern approach:
- for (auto it = t.m_thresholds.begin(), et = t.m_thresholds.end(); it != et;) { - extended_number b(*it); - o << b; - ++it; - if (it != t.m_thresholds.end()) { - o << ","; - } + bool first = true; + for (const auto& val : t.m_thresholds) { + if (!first) o << ","; + o << extended_number(val); + first = false; }src/crab/split_dbm.hpp (1)
Line range hint
114-120: Consider markingdiffcsts_of_lin_leqas const.Similar to
diffcsts_of_assign, this method appears to be a pure computation that converts linear inequalities into difference constraints without modifying the object's state. Consider adding theconstqualifier to maintain consistency with the PR's const-correctness objective.void diffcsts_of_lin_leq(const linear_expression_t& exp, /* difference constraints */ std::vector<diffcst_t>& csts, /* x >= lb for each {x,lb} in lbs */ std::vector<std::pair<variable_t, Weight>>& lbs, /* x <= ub for each {x,ub} in ubs */ - std::vector<std::pair<variable_t, Weight>>& ubs) const; + std::vector<std::pair<variable_t, Weight>>& ubs) const;src/crab/wto.cpp (1)
Line range hint
1-289: Consider additional const-correctness improvements throughout the file.While the current changes are good, there are more opportunities to improve const-correctness:
- Make print_visitor methods const as they don't modify the visitor's state:
- void operator()(const label_t& label) { o << label; } + void operator()(const label_t& label) const { o << label; } - void operator()(const wto_cycle_t& cycle) + void operator()(const wto_cycle_t& cycle) const
- Add const to wto_builder_t methods that don't modify member state:
- void push_successors(const label_t& vertex, wto_partition_t& partition, + void push_successors(const label_t& vertex, wto_partition_t& partition, const std::weak_ptr<wto_cycle_t>& containing_cycle) const;
- Consider making some helper methods const where appropriate:
- std::vector<label_t> collect_heads(const label_t& label) + std::vector<label_t> collect_heads(const label_t& label) constThese changes would further align with the PR's objective of improving const-correctness across the codebase.
src/asm_ostream.cpp (2)
Line range hint
229-238: Consider using structured bindings for more concise code.The code could be more readable using C++17's structured bindings.
- auto single = std::ranges::find_if(call.singles, [r](const ArgSingle arg) { return arg.reg.v == r; }); + if (auto single = std::ranges::find_if(call.singles, [r](const auto& arg) { return arg.reg.v == r; }); if (single != call.singles.end()) { if (r > 1) { os_ << ", "; } os_ << *single; continue; } - auto pair = std::ranges::find_if(call.pairs, [r](const ArgPair arg) { return arg.mem.v == r; }); + if (auto pair = std::ranges::find_if(call.pairs, [r](const auto& arg) { return arg.mem.v == r; }); if (pair != call.pairs.end()) {Also applies to: 239-248
Line range hint
272-285: LGTM! Consider minor string optimization.The const qualification is correct. A minor optimization could be made to avoid string allocation for the sign.
- const string sign = offset > 0 ? "+" : ""; - const string target = sign + std::to_string(offset) + " <" + to_string(b.target) + ">"; + const std::string_view sign = offset > 0 ? "+" : ""; + std::string target; + target.reserve(32); // Reserve space for typical case + target += sign; + target += std::to_string(offset); + target += " <"; + target += to_string(b.target); + target += ">";src/asm_files.cpp (1)
Line range hint
342-347: Fix const qualifier in the second loop.While the first loop correctly uses
const auto&as it only reads the map descriptors, the second loop modifies the map descriptors (updatingoriginal_fdandinner_map_fd). Therefore, the second loop should not use a const reference.Apply this diff to fix the issue:
// Gather the typeids for each map and assign a pseudo-fd to each map. for (const auto& map_descriptor : info.map_descriptors) { if (!type_id_to_fd_map.contains(map_descriptor.original_fd)) { type_id_to_fd_map[map_descriptor.original_fd] = pseudo_fd++; } } // Replace the typeids with the pseudo-fds. - for (const auto& map_descriptor : info.map_descriptors) { + for (auto& map_descriptor : info.map_descriptors) { map_descriptor.original_fd = type_id_to_fd_map[map_descriptor.original_fd]; if (map_descriptor.inner_map_fd != DEFAULT_MAP_FD) { map_descriptor.inner_map_fd = type_id_to_fd_map[map_descriptor.inner_map_fd]; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (33)
src/asm_cfg.cpp(7 hunks)src/asm_files.cpp(3 hunks)src/asm_marshal.cpp(1 hunks)src/asm_ostream.cpp(13 hunks)src/asm_parse.cpp(1 hunks)src/assertions.cpp(2 hunks)src/crab/array_domain.cpp(3 hunks)src/crab/cfg.hpp(7 hunks)src/crab/fwd_analyzer.cpp(8 hunks)src/crab/split_dbm.cpp(20 hunks)src/crab/split_dbm.hpp(1 hunks)src/crab/thresholds.cpp(5 hunks)src/crab/thresholds.hpp(3 hunks)src/crab/type_domain.cpp(1 hunks)src/crab/wto.cpp(2 hunks)src/crab/wto.hpp(1 hunks)src/crab_utils/adapt_sgraph.hpp(1 hunks)src/crab_utils/debug.cpp(1 hunks)src/crab_utils/graph_ops.hpp(22 hunks)src/crab_utils/stats.cpp(4 hunks)src/crab_utils/stats.hpp(1 hunks)src/crab_verifier.cpp(3 hunks)src/linux/gpl/spec_prototypes.cpp(2 hunks)src/linux/linux_platform.cpp(7 hunks)src/main/check.cpp(3 hunks)src/main/linux_verifier.cpp(1 hunks)src/main/run_yaml.cpp(1 hunks)src/test/conformance_check.cpp(1 hunks)src/test/ebpf_yaml.cpp(1 hunks)src/test/test_conformance.cpp(2 hunks)src/test/test_marshal.cpp(6 hunks)src/test/test_verify.cpp(1 hunks)src/test/test_wto.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (4)
src/crab/fwd_analyzer.cpp (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#745
File: src/crab/fwd_analyzer.hpp:14-14
Timestamp: 2024-11-05T20:59:02.521Z
Learning: When reviewing changes, avoid suggesting documentation additions for pre-existing functions (e.g., `run_forward_analyzer` in `src/crab/fwd_analyzer.hpp`) if they are outside the scope of the current PR.
src/crab/split_dbm.cpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#689
File: src/crab/split_dbm.cpp:910-910
Timestamp: 2024-09-26T00:57:58.072Z
Learning: Avoid suggesting changes that are unrelated to the current PR.
src/crab_utils/adapt_sgraph.hpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#689
File: src/crab_utils/adapt_sgraph.hpp:148-162
Timestamp: 2024-09-26T00:51:12.799Z
Learning: In `src/crab_utils/adapt_sgraph.hpp`, within the `AdaptGraph` class, the `Weight` type is intentionally set to `number_t`, and this decision has already been made.
src/crab_verifier.cpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#716
File: src/crab_verifier.cpp:115-115
Timestamp: 2024-10-05T18:27:27.631Z
Learning: In `src/crab_verifier.cpp` and throughout the codebase, it's acceptable to use `const` qualifiers on value parameters to indicate that the parameter is not modified within the function.
🪛 cppcheck
src/crab_utils/debug.cpp
[style] 12-12: The function 'CrabEnableWarningMsg' is never used.
(unusedFunction)
src/crab_utils/stats.cpp
[style] 98-98: The function 'count_max' is never used.
(unusedFunction)
[style] 102-102: The function 'uset' is never used.
(unusedFunction)
[style] 121-121: The function 'PrintBrunch' is never used.
(unusedFunction)
src/linux/gpl/spec_prototypes.cpp
[style] 2291-2291: The function 'get_helper_prototype_linux' is never used.
(unusedFunction)
src/test/test_conformance.cpp
[performance] 9-9: Function parameter 'filename' should be passed by const reference.
(passedByValue)
[performance] 10-10: Function parameter 'expected_reason' should be passed by const reference.
(passedByValue)
🔇 Additional comments (86)
src/crab_utils/debug.cpp (1)
12-12: LGTM! The const addition aligns with the PR objectives.
The change improves const-correctness without affecting functionality.
Let's verify the function usage across the codebase:
✅ Verification successful
Function is actively used and properly declared
The function is:
- Declared in
src/crab_utils/debug.hpp - Implemented in
src/crab_utils/debug.cpp - Used in
src/main/check.cpp - Header is included by multiple core components
The const addition is safe and the function serves an active role in the codebase, particularly in warning message control during checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if CrabEnableWarningMsg is used in the codebase
# Note: Looking in both source files and headers as it might be used in inline functions
# Search for function usage
echo "Searching for CrabEnableWarningMsg usage..."
rg "CrabEnableWarningMsg" --type cpp --type hpp
# Search for header includes to understand potential usage
echo "Checking header inclusion..."
rg "debug\.hpp" --type cpp --type hpp
Length of output: 307
Script:
#!/bin/bash
# Let's try a different approach using more generic file patterns
# Search for function usage in all C++ related files
echo "Searching for CrabEnableWarningMsg usage..."
rg "CrabEnableWarningMsg" -g "*.{cpp,hpp,h,cc}"
# Search for header includes
echo "Checking header inclusion..."
rg "debug\.hpp" -g "*.{cpp,hpp,h,cc}"
# Also check if the function is declared in a header file
echo "Checking function declaration in headers..."
fd -e hpp -e h | xargs grep -l "CrabEnableWarningMsg"
Length of output: 1001
🧰 Tools
🪛 cppcheck
[style] 12-12: The function 'CrabEnableWarningMsg' is never used.
(unusedFunction)
src/crab_utils/stats.hpp (1)
27-27: LGTM! Good improvement to const-correctness.
The addition of const to toSeconds() is appropriate as this method likely performs a calculation without modifying the object's state, similar to other const methods in the class like getTimeElapsed() and Print().
Let's verify if there are any other similar methods in related files that could benefit from const-correctness:
✅ Verification successful
Const-correctness change is safe and verified
The verification of the codebase shows that the toSeconds() method's const-correctness change aligns well with the codebase's patterns. The related utility files demonstrate consistent use of const-correctness, particularly for methods that perform calculations or return values without modifying state. For example:
- Time/measurement related methods in the codebase are consistently marked as const
- Similar utility methods like
narrow(),size(), and various getters follow the same const pattern - No potential conflicts or inconsistencies were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for other non-const methods in related files that might benefit from const-correctness
# Focusing on time/measurement related methods that likely don't modify state
# Search for method declarations in related utility files
rg -g 'src/crab_utils/*' '^\s*\w+\s+\w+\([^)]*\)\s*[^;]*$' --type cpp
Length of output: 7403
src/main/linux_verifier.cpp (1)
15-15: 🧹 Nitpick (assertive)
LGTM! The const addition aligns with the PR objectives.
While the const qualifier is technically redundant here since the parameter is passed by value (meaning the function already receives a copy), it's still a good practice as it clearly documents the intent that the command should not be modified. This change maintains consistency with similar const-correctness improvements across the codebase.
Consider also marking the function itself as const since it doesn't modify any class state:
-static int do_bpf(const bpf_cmd cmd, union bpf_attr& attr)
+static int do_bpf(const bpf_cmd cmd, union bpf_attr& attr) constLikely invalid or redundant comment.
src/crab/thresholds.hpp (2)
42-42: LGTM! Excellent optimization.
The change to pass by const reference is a good optimization that prevents unnecessary copying while maintaining const-correctness. This follows modern C++ best practices for parameter passing of non-trivial types.
64-64: LGTM! Const qualifier addition is appropriate.
The const qualifier for max_size is a good addition, and keeping cfg as a non-const reference is correct since it's stored as a reference member. This maintains both safety and flexibility where needed.
src/crab/thresholds.cpp (3)
58-62: LGTM: Good const-correctness improvements.
Appropriate use of const for variables and references, preventing unintended modifications.
71-71: LGTM: Consistent const-correctness improvements.
Good use of const references throughout the method, particularly in loop variables and parameter references.
Also applies to: 76-76, 85-85
92-92: LGTM: Excellent use of structured binding with const reference.
The use of structured binding with const auto& is a perfect example of modern C++ features combined with const-correctness.
src/test/test_wto.cpp (2)
32-32: LGTM! Appropriate use of const qualifier
The const qualifier is correctly applied here as the wto object is only used for verification and is not modified after construction.
60-60: LGTM! Consistent const usage
The const qualifier is appropriately applied, maintaining consistency with the previous test case and preventing unintended modifications.
src/crab_utils/stats.cpp (5)
40-42: LGTM: Appropriate const usage
The addition of const for variable r correctly indicates its immutable nature after initialization.
75-75: LGTM: Good const method qualification
The method is correctly marked as const since it doesn't modify object state. The simplified single-line implementation improves readability.
78-81: LGTM: Appropriate const usage for local variables
The addition of const qualifiers to time, h, m, and s correctly indicates their immutable nature after initialization.
112-127: LGTM: Good use of const references in loops
The use of const auto& in the loops is a good practice as it:
- Prevents accidental modification of elements during iteration
- Avoids unnecessary copying
- Clearly indicates read-only intent
Note: Static analysis indicates PrintBrunch is currently unused. Consider documenting its purpose if it's meant for future use.
🧰 Tools
🪛 cppcheck
[style] 121-121: The function 'PrintBrunch' is never used.
(unusedFunction)
132-132: LGTM: Appropriate const parameter qualification
The reset parameter is correctly marked as const since it's only used for condition checking and shouldn't be modified.
src/crab/fwd_analyzer.cpp (5)
Line range hint 34-39: LGTM: Appropriate use of const reference in range-based for loop
The change to use const auto& for the loop variable is a good improvement that prevents unnecessary copying while ensuring immutability.
77-78: LGTM: Appropriate const-qualification of function parameters
The addition of const qualifiers to the parameters of extrapolate() and refine() functions is correct as these functions don't modify their inputs.
Also applies to: 88-88
143-150: LGTM: Improved control flow with appropriate const usage
Good improvements:
- Simplified skip logic while maintaining the same behavior
- Appropriate use of const for the pre variable
Line range hint 157-177: LGTM: Appropriate const usage for loop head variable
The addition of const qualifier to the head variable is correct as it's used as a read-only value throughout its scope.
Line range hint 189-193: LGTM: Consistent use of const references in range-based loops
The changes to use const auto& in both loops are appropriate and consistent with the const-correctness improvements throughout the file.
Also applies to: 210-214
src/crab/split_dbm.hpp (2)
109-109: LGTM! Good const-correctness improvement.
Adding the const qualifier to diffcsts_of_assign is appropriate as this method performs a read-only computation to generate difference constraints from assignments without modifying the object's state. This change aligns well with the PR's objective of improving const-correctness.
109-109: Verify const method implementations.
Let's verify that const methods don't modify state through mutable members or thread-local state.
✅ Verification successful
Based on the analysis of the const methods and their implementations, I can now generate the final response:
Const correctness is properly maintained
The const methods in SplitDBM are correctly implemented:
- No mutable members were found that could bypass const-correctness
- The only thread_local state is managed through
GraphOps::clear_thread_local_state(), which is a static method - All const methods perform read-only operations on their internal state
- Helper functions are properly marked static and don't modify object state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check const method implementations and usages
# Test 1: Find const method declarations
echo "=== Const method declarations ==="
rg "const\s*;" src/crab/split_dbm.hpp
# Test 2: Check for mutable members that might bypass const
echo -e "\n=== Mutable members ==="
rg "mutable" src/crab/split_dbm.hpp
# Test 3: Look for thread_local state that might be modified
echo -e "\n=== Thread-local state ==="
rg "thread_local" src/crab/split_dbm.hpp
# Test 4: Find const method implementations
echo -e "\n=== Const method implementations ==="
rg "const\s*{" src/crab/split_dbm.cpp
Length of output: 1636
Script:
#!/bin/bash
# Let's check const method implementations with a corrected pattern and look for potential state modifications
# Check const method implementations with proper regex
echo "=== Const method implementations ==="
rg "const\s+\{|const\s*$" src/crab/split_dbm.hpp src/crab/split_dbm.cpp -A 5
# Check if GraphOps has any mutable state that might affect const correctness
echo -e "\n=== GraphOps implementation ==="
rg "class GraphOps" -A 20 src/crab/graph_ops.hpp
# Look for any static/global variables that might be modified
echo -e "\n=== Static/Global state ==="
rg "static\s+(thread_local\s+)?\w+" src/crab/split_dbm.hpp src/crab/split_dbm.cpp
# Check if AdaptGraph has mutable state
echo -e "\n=== AdaptGraph implementation ==="
rg "class AdaptGraph" -A 20 src/crab/adapt_graph.hpp
Length of output: 9416
src/crab_verifier.cpp (3)
111-111: LGTM: Appropriate use of const reference in range-based for loop
The addition of const qualifier to the loop variable is correct as the loop only reads from the elements without modifying them. This change improves safety while maintaining efficiency through reference semantics.
123-123: LGTM: Proper const reference usage for map iteration
The addition of const qualifier is appropriate here as the loop only reads from the map entries for printing. This change ensures immutability while avoiding unnecessary copying of map elements.
203-203: LGTM: Appropriate const reference usage for invariant table iteration
The addition of const qualifier is correct as the loop only reads from the invariant table to construct a new map. This change ensures safety and efficiency through const reference semantics.
src/linux/linux_platform.cpp (7)
134-134: LGTM: Appropriate const qualifier addition
The const qualifier for index is appropriate as it's only used for lookup and comparison operations.
147-149: LGTM: Well-structured const parameter qualifiers
The const qualifiers are correctly applied to read-only parameters while maintaining mutability for map_descriptors that needs modification.
159-159: LGTM: Efficient const reference iteration
Using const auto& for iteration prevents unnecessary copying while ensuring immutability of the map definitions.
176-176: LGTM: Appropriate const usage for map index
The const qualifier for inner is appropriate as it's only used for comparison and array access operations.
186-186: LGTM: Proper const usage in syscall wrapper
The const qualifier for cmd is appropriate while maintaining mutability for attr that needs to be modified by the syscall.
193-196: LGTM: Comprehensive const usage in map creation
The const qualifiers are appropriately applied to all input parameters and local variables that don't require modification.
Also applies to: 208-208
227-227: LGTM: Proper const qualifier for descriptor lookup
The const qualifier for map_fd is appropriate as it's only used for lookup operations.
src/main/check.cpp (2)
157-159: LGTM! Good use of const reference.
The use of const auto& for the loop variable is appropriate here as it prevents unnecessary copying and ensures immutability while iterating over the group names.
160-162: LGTM! Consistent use of const reference.
The use of const auto& maintains consistency with the previous loop and is appropriate for read-only iteration over the group names.
src/test/test_conformance.cpp (2)
Line range hint 20-27: LGTM! Good use of const references
The loop implementation correctly uses const auto& for iteration and proper reference destructuring of the tuple, which aligns well with the PR's const-correctness objectives.
Line range hint 29-208: LGTM! Well-structured test organization
The test macros and test cases are well-organized and properly handle different test scenarios. The implementation aligns with the testing framework's requirements and doesn't require any const-related modifications.
🧰 Tools
🪛 cppcheck
[performance] 9-9: Function parameter 'filename' should be passed by const reference.
(passedByValue)
[performance] 10-10: Function parameter 'expected_reason' should be passed by const reference.
(passedByValue)
src/assertions.cpp (1)
50-50: LGTM: Clean return statement consolidation.
The simplified return statement improves readability while maintaining the same functionality.
src/asm_cfg.cpp (7)
53-53: LGTM: Appropriate const qualification
The stack_frame_prefix is only used for reading after initialization, making const qualification appropriate here.
73-73: LGTM: Proper const usage for immutable label
The label is only used for lookups after creation, making const qualification appropriate and preventing accidental modifications.
122-123: LGTM: Appropriate const usage for stack frame handling
Both changes enhance code safety:
stack_frame_depthis computed once and only used for comparison- Loop variable
macro_labelis only used for reading, making const reference appropriate
183-183: LGTM: Proper const reference in loop
Using const auto& for the loop variable is appropriate as it's only used for reading and prevents unnecessary copying of tuple elements.
235-235: LGTM: Appropriate const usage in CFG iteration
Using const auto& for CFG node iteration prevents accidental modification and avoids unnecessary copying of the label-block pairs.
262-262: LGTM: Proper const usage in jumps iteration
Using const auto& for iterating over jumps is appropriate as the elements are only used for reading and constructing new blocks.
372-372: LGTM: Appropriate const qualification for WTO
Making the WTO object const is appropriate as it's only used for iteration over loop heads and is never modified after construction.
src/test/ebpf_yaml.cpp (3)
32-34: LGTM! Appropriate const qualification.
The addition of const to the platform_specific_type parameter is correct as the value is only used for reading and isn't modified within the function.
36-38: LGTM! Appropriate const qualification.
The addition of const to the n parameter is correct as the value is only used for reading and isn't modified within the function.
40-40: LGTM! Appropriate const qualification.
The addition of const to the n parameter is correct as the value is only used for reading and isn't modified within the function.
src/asm_parse.cpp (1)
96-96: LGTM! Good const-correctness improvement.
Adding const qualifier to the lddw parameter is a good practice as it explicitly indicates that the parameter is read-only and prevents accidental modifications. This change aligns well with the PR's objective of improving const-correctness.
src/asm_ostream.cpp (4)
Line range hint 24-35: LGTM! Appropriate const qualification.
The const qualification for the kind parameter is correct as this is a read-only operation converting enum values to strings.
199-200: LGTM! Appropriate const qualification.
The const qualification for the is64 parameter is correct as this is a read-only operation formatting register names.
Line range hint 494-513: LGTM! Good use of const auto.
The implementation correctly uses const auto in the range-based for loop, consistent with the const-correctness objective.
Line range hint 517-526: LGTM! Consistent implementation.
The implementation maintains consistency with the basic_block_t operator and correctly uses const auto.
src/crab/cfg.hpp (5)
148-148: Good modernization using std::ranges::move!
The change from std::move to std::ranges::move is a good improvement that leverages modern C++20 ranges functionality, providing better safety and clarity for range operations.
245-246: LGTM: Simplified iterator type definitions
The removal of redundant typename keywords aligns with modern C++17 practices while maintaining the same functionality.
344-345: LGTM: Improved const-correctness in loop
Using const auto& in the range-based for loop is a good practice that prevents accidental modifications and avoids unnecessary copying, aligning well with the PR's const-correctness objective.
460-460: LGTM: Better set membership check
Using contains() instead of count() > 0 is more idiomatic and efficient for set membership testing, as it better expresses the intent and avoids unnecessary counting.
575-575: LGTM: Consistent use of contains()
The consistent replacement of count() > 0 with contains() across these methods improves both readability and performance of set membership testing.
Also applies to: 578-579, 593-594, 599-599
src/asm_files.cpp (2)
235-235: LGTM! Good use of const reference.
The use of const auto& for the loop variable is appropriate here as it prevents modification of the relocation data and avoids unnecessary copying of the function_relocation struct.
268-268: LGTM! Appropriate use of const reference for BTF map data.
The use of const auto& for the loop variable is correct as it prevents modification of the BTF map data and avoids unnecessary copying of potentially large map structures.
src/crab/array_domain.cpp (3)
229-229: LGTM! Good use of const reference.
The change to const auto& is appropriate here as it prevents unnecessary copying while ensuring the loop variable cannot be modified, which is the desired behavior when only reading from the container.
293-293: LGTM! Consistent const usage.
Using const auto& here maintains consistency with the previous change and is the correct choice for read-only iteration over container elements.
Line range hint 550-558: LGTM! Well-structured const iteration.
The change to const auto& in this loop is appropriate as:
- It prevents unnecessary copying of cell objects
- It clearly communicates the read-only nature of the iteration
- It's consistent with modern C++ best practices
src/test/test_verify.cpp (1)
55-55: LGTM! Good const-correctness improvement.
The change from auto& to const auto& is a good improvement since the loop variable is only used for reading. This change:
- Prevents accidental modifications
- Makes the intent clearer
- Follows modern C++ best practices
src/crab_utils/graph_ops.hpp (2)
107-107: LGTM! Proper use of typename for dependent types.
The addition of the typename keyword for dependent types within templates is correct and necessary. This change improves type safety and clarity in template code by explicitly indicating that these are dependent types rather than static members or variables.
Also applies to: 140-140, 166-166, 205-205, 498-502
592-592: LGTM! Consistent use of const in range-based for loops.
The code demonstrates good practices in terms of const-correctness:
- Appropriate use of
const autofor range-based for loops over graph edges - Proper use of
const auto&for structured bindings to avoid unnecessary copies - Consistent with the PR's objective of making variables const by default
Also applies to: 609-609, 629-629, 752-752, 777-777, 804-804, 873-873, 944-944, 970-970, 1017-1017, 1096-1096, 1113-1113, 1142-1142, 1162-1162, 1179-1181, 1205-1205, 1213-1213
src/crab/split_dbm.cpp (13)
82-82: LGTM: Good use of const reference in range-based for loop.
The const reference prevents accidental modifications to the loop variables and avoids unnecessary copying.
125-127: LGTM: Appropriate use of const reference for loop variables.
The const reference is correctly used here as the loop variables are only read within the loop body.
158-158: LGTM: Proper const-correctness in range-based for loop.
The const reference is appropriate as the loop variables are only used for reading.
199-206: LGTM: Consistent const-correctness in variable declarations.
Both the variable initialization and loop variable declaration properly enforce const-correctness.
215-217: LGTM: Proper const usage in variable declarations.
Both the variable initialization and loop variable declaration correctly use const where appropriate.
224-234: LGTM: Consistent const usage across multiple loops.
All loop variable declarations properly use const references, maintaining consistent const-correctness throughout the nested and sequential loops.
244-246: LGTM: Proper const usage in loop and variable declarations.
Both the loop variables and local variable declarations correctly use const qualifiers.
258-260: LGTM: Consistent const usage in loop and variable declarations.
The const qualifiers are correctly used for both the loop variables and local variable declaration.
272-276: LGTM: Proper const usage throughout the code segment.
The const qualifiers are correctly used for both loop variables and local variable declarations.
883-887: LGTM: Consistent const usage in parallel loops.
Both loops correctly use const references for their variables, maintaining consistent const-correctness.
Line range hint 1209-1213: LGTM: Proper const usage in variable declarations.
The const qualifiers are correctly used for both the local variable and loop variables.
1310-1310: LGTM: Proper const usage in function parameter.
The const reference is correctly used for the function parameter that is only read within the function.
1105-1108: 🧹 Nitpick (assertive)
Consider passing by value for small types.
While the const qualifier is correct, since variable_t is likely a small type (possibly an enum or small struct), passing by value might be more efficient than by reference.
- for (const auto v : variables) {
+ for (const auto& v : variables) {Likely invalid or redundant comment.
src/test/test_marshal.cpp (6)
Line range hint 340-344: LGTM: Good use of const for loop variable.
The change to const auto for the loop variable improves type safety by preventing accidental modifications during iteration.
Line range hint 350-354: LGTM: Consistent const usage for loop variable.
The change maintains consistency with the previous loop and properly enforces immutability of the loop variable.
375-378: LGTM: Appropriate const usage for endian operations loop.
The const qualifier correctly enforces immutability for the loop variable in this endian operations test.
385-388: LGTM: Proper const usage for swap operations loop.
The const qualifier appropriately prevents modification of the loop variable during iteration over swap operations.
Line range hint 406-413: LGTM: Well-applied const qualifier for condition operations.
The const qualifier correctly enforces immutability while preserving the error checking functionality.
Line range hint 415-422: LGTM: Consistent const usage for immediate-based operations.
The const qualifier is appropriately applied, maintaining consistency with the register-based operations loop above.
src/linux/gpl/spec_prototypes.cpp (2)
Line range hint 2277-2287: LGTM! Good const-correctness improvement.
The addition of const to the parameter is appropriate as the value is only read within the function. The function's logic for validating helper function usability remains correct.
Line range hint 2291-2296: LGTM! Good const-correctness improvement.
The addition of const to the parameter is appropriate as the value is only read within the function.
Static analysis indicates this function is never used. Let's verify its usage:
🧰 Tools
🪛 cppcheck
[style] 2291-2291: The function 'get_helper_prototype_linux' is never used.
(unusedFunction)
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
src/crab_utils/adapt_sgraph.hpp (1)
Line range hint
231-233: Potential undefined behavior due to optional dereference inedge_valIn the
edge_valfunction, the expression*_succs[s].lookup(d)dereferences anstd::optionalwithout checking if it contains a value. If the edge does not exist, this could lead to undefined behavior.Consider modifying the function to handle the case where the edge may not exist or ensure that a precondition is in place to guarantee the edge's existence before calling
edge_val.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
src/assertions.cpp(5 hunks)src/crab/cfg.hpp(9 hunks)src/crab_utils/adapt_sgraph.hpp(1 hunks)src/main/check.cpp(6 hunks)src/test/test_conformance.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/crab_utils/adapt_sgraph.hpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#689
File: src/crab_utils/adapt_sgraph.hpp:148-162
Timestamp: 2024-09-26T00:51:12.799Z
Learning: In `src/crab_utils/adapt_sgraph.hpp`, within the `AdaptGraph` class, the `Weight` type is intentionally set to `number_t`, and this decision has already been made.
🔇 Additional comments (18)
src/main/check.cpp (4)
24-25: LGTM: Appropriate const-correctness for hash function parameters.
The change from char* to const char* correctly reflects that these pointers are used only for reading in boost::hash_range.
56-56: LGTM: Proper const reference for string parameter.
The change to const std::string& is appropriate as the parameter is only read, preventing unnecessary copies.
157-157: LGTM: Proper const references in range-based for loops.
The use of const auto& for loop variables is appropriate as they are only used for reading, preventing unnecessary string copies.
Also applies to: 160-160
253-254: LGTM: Appropriate const qualification for cfg variables.
The addition of const to the cfg variables correctly reflects their immutable nature after creation.
Also applies to: 268-269
src/test/test_conformance.cpp (3)
9-10: LGTM: Parameter optimization implemented as suggested
The changes align with the previous review suggestion to optimize parameter passing with const references.
13-14: LGTM: Good use of const for local variables
Making extension and plugin_path const is appropriate as they are not modified after initialization. This improves code safety by preventing accidental modifications.
20-20: LGTM: Optimized loop with const reference
Using const auto& for the loop variable is appropriate as it prevents unnecessary copying while ensuring the loop variable cannot be accidentally modified.
src/assertions.cpp (2)
Line range hint 40-232: LGTM! Excellent const-correctness improvements.
The consistent use of const references for operator parameters is a great improvement that:
- Prevents accidental modifications
- Enables compiler optimizations
- Aligns with modern C++ best practices
306-307: LGTM! Good use of const references in loop variables.
The use of const references in the loop variables prevents unnecessary copying while maintaining immutability, which aligns well with the PR's objectives.
src/crab/cfg.hpp (6)
148-148: Good use of modern C++ ranges!
The change from std::move to std::ranges::move is a good modernization that provides better range handling and clearer semantics.
245-246: LGTM: Simplified iterator type definitions
Removing redundant typename keywords improves readability while maintaining the same functionality.
390-390: LGTM: Efficient set construction
Good optimization using label iterators directly in the set constructor.
460-460: LGTM: Better set membership check
Using contains instead of count > 0 is more idiomatic and potentially more efficient for set membership testing.
559-561: Changes address previous review feedback
The const-correctness improvements match the suggestions from previous reviews.
575-579: LGTM: Consistent use of contains
Good consistency in replacing all count > 0 checks with contains throughout the codebase.
Also applies to: 593-594, 599-599
src/crab_utils/adapt_sgraph.hpp (3)
181-183: Good use of const references in loop variables
The loop correctly uses const auto& e, which prevents unnecessary copying and aligns with best practices for efficient iteration.
Line range hint 327-329: Efficient iteration with structured bindings
Using const auto& [key, val] in the loop within forget(vert_id v) efficiently iterates over the elements without unnecessary copies.
Line range hint 395-406: Acknowledgment of non-const operator<<
Understanding that g cannot be marked const in the operator<< function due to existing complexities is acceptable given the comment provided.
Signed-off-by: Elazar Gershuni <[email protected]>
Also some modernization. Essentially, variable, parameters and methods should be const by default.
Summary by CodeRabbit