Tidy #883
Tidy #883
Conversation
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Lookup now always returns a pointer and does not accept one. In addition to cleanup, this also runs faster Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (30)
src/asm_cfg.cpp(2 hunks)src/asm_ostream.cpp(1 hunks)src/asm_unmarshal.cpp(1 hunks)src/asm_unmarshal.hpp(1 hunks)src/cfg/wto.cpp(1 hunks)src/crab/array_domain.cpp(1 hunks)src/crab/bitset_domain.hpp(3 hunks)src/crab/ebpf_checker.cpp(1 hunks)src/crab/ebpf_domain.cpp(1 hunks)src/crab/ebpf_domain.hpp(1 hunks)src/crab/finite_domain.cpp(1 hunks)src/crab/finite_domain.hpp(1 hunks)src/crab/interval.hpp(1 hunks)src/crab/linear_expression.hpp(3 hunks)src/crab/split_dbm.cpp(2 hunks)src/crab/split_dbm.hpp(1 hunks)src/crab/thresholds.hpp(1 hunks)src/crab_utils/adapt_sgraph.hpp(6 hunks)src/crab_utils/graph_ops.hpp(21 hunks)src/crab_utils/num_extended.hpp(2 hunks)src/crab_utils/stats.cpp(1 hunks)src/linux/gpl/spec_prototypes.cpp(2 hunks)src/linux/linux_platform.cpp(1 hunks)src/main/check.cpp(1 hunks)src/main/linux_verifier.cpp(1 hunks)src/main/linux_verifier.hpp(2 hunks)src/main/memsize_linux.hpp(1 hunks)src/string_constraints.hpp(1 hunks)src/test/test_conformance.cpp(1 hunks)src/test/test_verify.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/asm_unmarshal.hpp (2)
src/asm_unmarshal.cpp (5)
imm(238-238)imm(238-238)imm(240-240)imm(240-240)imm(489-489)src/asm_parse.cpp (2)
imm(99-115)imm(99-99)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (66)
src/asm_cfg.cpp (2)
137-139: Stack frame comment punctuation revised. Improves inline documentation without altering logic.
222-222: Comma added to introductory comment. Clearly sets off the clause before instruction listing.src/asm_unmarshal.cpp (1)
51-57: Mark exceptions asfinalappropriately.Preventing further inheritance of these exception types aligns with design intent and may improve performance by enabling devirtualization. No functional impact detected.
src/asm_unmarshal.hpp (1)
26-26: Rename parameter to match implementation.Changing
functoimmaligns the header declaration with the internal API inasm_unmarshal.cpp. No further updates required.src/cfg/wto.cpp (1)
66-66: Remove redundant semicolon in initializer list
Trailing semicolon in theVisitArgsconstructor initializer list has been removed, improving syntax cleanliness without impacting functionality.src/string_constraints.hpp (1)
20-20: Eliminate superfluous semicolon in constructor definition
Removed the redundant semicolon at the end ofStringInvariant’s explicit constructor, aligning with code style.src/crab_utils/stats.cpp (1)
81-81: Adopt C++-style cast for clarity
Replaced the C-style cast withstatic_cast<float>when computings, ensuring type-safe conversion and consistent modern C++ practice.src/asm_ostream.cpp (1)
473-473: Enforce immutability withconstqualifier
Addedconstto theoffsetvariable inprint(Deref const&)to clarify intent and prevent accidental modification.src/main/memsize_linux.hpp (1)
23-23: Markpage_size_kbasconst long
Scopedpage_size_kbdeclaration inside the block as aconst longcommunicates immutability and preserves readability.src/crab/finite_domain.hpp (1)
128-128: Fixed parameter naming inbitwise_xormethod.The parameter name was corrected from
lhssstolhss, matching the naming pattern used in other similar methods.src/crab_utils/num_extended.hpp (5)
129-129: Improved parameter passing in function pointer signature.Changed the function pointer signature to use
const Number&instead ofNumberby value, which avoids unnecessary copying ofNumberobjects.
147-148: Updated lambda to use const references.Updated the lambda in
operator/to use const references for parameters, matching the updated function pointer signature.
152-153: Updated lambda to use const references.Updated the lambda in
operator%to use const references for parameters, matching the updated function pointer signature.
159-162: Updated lambda to use const references.Updated the lambda in
udivto use const references for parameters, matching the updated function pointer signature.
168-171: Updated lambda to use const references.Updated the lambda in
uremto use const references for parameters, matching the updated function pointer signature.src/test/test_conformance.cpp (1)
11-11: Added const qualifier to local variable.The
test_filesvector is now properly marked as const since it's not modified after initialization.src/crab/thresholds.hpp (1)
37-37: Simplified parameter name inaddmethod.Changed parameter name from
v1to the more concisev.src/crab/split_dbm.hpp (1)
137-137: Added const qualifier to local variable pMaking the local variable immutable improves const-correctness and clarifies intent.
src/linux/linux_platform.cpp (1)
136-136: Improved array bounds check with std::sizeUsing
std::sizeinstead of manual array size calculation is safer and more idiomatic in modern C++. This eliminates potential errors from manual size calculations.src/main/check.cpp (1)
51-51: Modernized map key iterationUsing
std::views::keysrange adapter provides a cleaner way to iterate over map keys without creating unnecessary value bindings.src/crab/ebpf_domain.hpp (1)
62-62: Added const qualifier to widening_thresholds method.The method signature has been updated to be const-correct, and the unused parameter name
tshas been removed while keeping its type declaration.src/test/test_verify.cpp (1)
610-610: Improved function signature for test_analyze_thread.Added static linkage to limit visibility to the translation unit and added const qualifier to ProgramInfo parameter since it's only read, not modified.
src/crab/linear_expression.hpp (4)
14-14: Minor wording improvement in comment.Improved clarity in the comment by changing "just a coefficient" to "simply a coefficient".
29-29: Good use of const iterator.Added const qualifier to the iterator since it's only used for reading.
33-33: Better idiom for iterator dereferencing.Changed from
(*it).secondtoit->secondwhich is the more common C++ idiom.
158-158: Added const qualifier to local variable.Enhanced const-correctness by declaring the local variable as const.
src/crab/interval.hpp (1)
176-176: Made method const-correct.Added const qualifier to the
widening_thresholdsmethod which doesn't modify the object state.src/linux/gpl/spec_prototypes.cpp (2)
2088-2088: Changed to constexpr array.Converted
prototypesfrom const to constexpr, enabling compile-time evaluation.
2279-2279: Safer boundary check using std::size.Replaced manual array size calculation with
std::size(prototypes)for safer bounds checking.src/main/linux_verifier.cpp (1)
24-24: Improved const-correctness of options parameter.Changed
optionsfrom a mutable pointer to a pointer-to-const since the function doesn't modify the options.src/crab/split_dbm.cpp (3)
388-388: Improved semantics with named constantReplacing the magic number
-1with a named constantINVALID_VERTincreases code readability and maintainability.
390-390: Good initialization using the named constantUsing the named constant
INVALID_VERTfor initialization instead of the literal-1makes the code more consistent and self-documenting.
411-411: Better assertion checksUpdating assertions to check against the named constant
INVALID_VERTprovides clearer error messages and maintains consistency with the initialization.Also applies to: 415-415
src/crab/array_domain.cpp (1)
858-860: Simplified method implementationRefactoring
widening_thresholdsto delegate to the existingwidenmethod reduces code duplication and improves maintainability.src/main/linux_verifier.hpp (1)
20-20: Improved const-correctnessMaking
optionsparameter const clarifies that the function doesn't modify the options and prevents accidental mutation.Also applies to: 29-29
src/crab/bitset_domain.hpp (7)
19-19: Enhanced constructor signatureTaking the parameter by const reference prevents unnecessary copies and makes it clear the constructor doesn't modify the input.
63-65: Improved const-correctness and bounds handlingAdding const qualifier to the
lbparameter and using explicit cast for the bounds check enhances type safety.
80-86: Better type handlingAdding const qualifier to parameter and using static_cast for the return value provides clearer intent and safer type conversion.
88-90: Safer parameter handlingAdding const qualifier and explicit casting improves type safety when dealing with size bounds.
95-97: Consistent parameter handlingSimilar to other methods, adding const qualifier and proper type casting enhances safety and consistency.
102-102: Correct friend operator declarationUsing const reference for the parameter is appropriate since the stream operator doesn't modify the BitsetDomain.
109-110: Enhanced bounds checkingUsing explicit cast and comparing against the properly typed size improves type safety in the boundary check.
src/crab_utils/adapt_sgraph.hpp (10)
126-126: Good enhancement of const correctness.Using
const autohere guarantees that the iterator won't be modified, which is appropriate for this lookup operation.
161-161: Simplified constructor using C++ default.The default constructor is now explicitly marked as defaulted, improving code readability and maintainability.
356-356: Improved const correctness in loop variable.Making the loop variable const signals that it's not modified within the loop body.
368-368: Added const qualifier to loop variable.Same improvement as above - this makes the code's intent clearer.
392-397: Better API design using pointer returns instead of reference parameters.The simplified
lookupmethod now returns a pointer directly instead of using a mutable reference out parameter. This is clearer, more modern, and eliminates the need for theMutValRefclass.
400-405: Simplified const lookup method.The const version of
lookupis now consistent with the non-const version, improving API coherence.
424-429: Updated update_edge to use the new lookup API.This method now correctly uses the pointer-returning
lookupmethod, checking for non-null before dereferencing.
432-437: Updated set_edge to use the new lookup API.Similar to update_edge, this method now uses the improved lookup API.
440-440: Improved operator<< to take a const reference.Taking const reference for stream output operator is good practice since it doesn't modify the graph.
443-443: Added const qualifier to loop variable.Another instance of improving const correctness.
src/crab_utils/graph_ops.hpp (14)
48-53: Simplified GraphPerm::lookup to return a pointer.The lookup method now returns a pointer directly instead of using a reference parameter. This is consistent with the changes in AdaptGraph and provides a cleaner API.
285-297: Updated SubGraph::lookup to return pointers.Both const and non-const versions of lookup now return pointers rather than using out parameters, maintaining consistency with the rest of the codebase.
449-450: Simplified GraphRev::lookup methods.These lookup methods now return pointers directly, consistent with other graph classes.
575-577: Updated join method to use pointer-returning lookup.The join method now correctly checks for non-null pointers returned by lookup instead of boolean returns with reference parameters.
590-596: Updated meet method to use pointer-returning lookup.Similar to the join method, this now uses the new lookup API, checking for non-null pointers and dereferencing only when valid.
672-674: Improved const correctness in for-loops.Added const qualifiers to loop variables since they aren't modified within the loops.
Also applies to: 683-685
719-723: Added const qualifier to loop variable.Improved const correctness in this for-loop.
736-736: Added const qualifiers to local variables.Enhanced const correctness for variables that aren't modified after initialization.
Also applies to: 760-760
763-765: Added const qualifier to loop variable.Another instance of improved const correctness.
981-984: Improved const correctness in for-loops.Added const qualifiers to loop variables in the repair_potential method.
Also applies to: 1019-1021
1053-1055: Added const qualifier to loop variable.Enhanced const correctness in the close_after_assign_fwd method.
1104-1120: Updated close_over_edge to use pointer-returning lookup.This method now correctly uses the new lookup API, checking for non-null pointers before dereferencing.
1132-1137: Updated pointer check in close_over_edge.Continuing the update to use pointer-returning lookup consistently.
1148-1153: Final update to close_over_edge using pointer-returning lookup.The last instance of updating to the new lookup API in this method.
Signed-off-by: Elazar Gershuni <[email protected]>
Signed-off-by: Elazar Gershuni <[email protected]>
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
scripts/rename_classes.py(2 hunks)src/crab/bitset_domain.hpp(3 hunks)src/crab_utils/adapt_sgraph.hpp(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build_ubuntu (Release, tests)
- GitHub Check: build_windows (Release, tests)
- GitHub Check: build_windows (Debug, library)
- GitHub Check: build_ubuntu (Debug, tests)
- GitHub Check: build_windows (Release, library)
- GitHub Check: build_windows (Debug, tests)
- GitHub Check: Analyze (cpp)
- GitHub Check: build_ubuntu (Release)
- GitHub Check: build_ubuntu (Debug)
🔇 Additional comments (18)
scripts/rename_classes.py (2)
2-3: License header added properly.Good addition of copyright and license headers to maintain proper attribution.
103-104: Class naming consistency improved.The renamed class mappings now align better with the actual implementation in the C++ code. Using "Value" instead of "Elt" makes the purpose clearer, and using full words like "Iterator" instead of abbreviated "Iter" improves readability.
Also applies to: 112-112, 114-114
src/crab_utils/adapt_sgraph.hpp (13)
21-21: Type alias renamed for clarity.Changing from "EltIter" to "ValueIterator" improves code readability by using a more descriptive name.
27-40: Class name improved for consistency.Changing "KeyIter" to "KeyIterator" aligns with the naming pattern of using full words instead of abbreviations.
70-88: Class renamed for clarity.Renaming "EltRange" to "ValueRange" improves code clarity by using a more descriptive term for what the class represents.
90-108: Class renamed for consistency.Renaming "EltConstRange" to "ValueConstRange" maintains consistency with other renamed classes and uses a more descriptive term.
111-113: Method renamed to match class renaming.Changing "elts()" to "values()" aligns with the renamed classes and improves API consistency.
162-162: Constructor simplified.Using
= defaultfor the constructor is cleaner and follows modern C++ best practices.
227-253: Struct renamed for consistency.Renaming "edge_const_iter" to "EdgeConstIterator" follows the same naming convention as other iterators and improves code readability.
255-274: Struct renamed for consistency.Renaming "edge_const_range_t" to "EdgeConstRange" aligns with the naming pattern used throughout the codebase.
298-298: Type alias renamed for consistency.Changing "e_neighbour_const_range" to "ENeighbourConstRange" follows CamelCase naming convention used elsewhere.
418-423: Updated lookup usage for pointer return type.Code correctly updated to check for non-null pointers instead of boolean return values, maintaining the same logic with the improved API.
Also applies to: 426-431
343-346: Improved const-correctness in range-based for loops.Adding
constto range-based for loop variables prevents accidental modification and better expresses intent.
434-434: Enhanced const-correctness in operator<<.Changed parameter to
const AdaptGraph&to properly indicate that the graph is not modified by the operator.
437-437: Added const-correctness to loop variable.Using
const VertId vin the range-based for loop improves code safety and prevents accidental modification.src/crab/bitset_domain.hpp (3)
19-19: Explicit constructor with const-reference parameter
Marking the constructorexplicitand takingbits_tby const-reference is correct: it prevents unintended implicit conversions and avoids unnecessary copies of the bitset.
86-92:all_num_widthsignature and implementation are consistent
Returning anintand casting the size difference avoids unsigned-to-signed surprises. The clamping logic handles out‐of‐boundslbcorrectly by returning zero whenlb >= EBPF_TOTAL_STACK_SIZE.
108-108:operator<<friend signature is correct
Declaring the stream‐inserter with aconst BitsetDomain¶meter aligns with usual conventions and avoids unnecessary copies.
Pull Request Test Coverage Report for Build 15175328890Details
💛 - Coveralls |
Mostly cleanup; the commit titles should be self-explanatory.
One significant cleanup is replacing
MutValRefclass with a simpleWeight*, and always returning Weight* from lookup functions instead of passing an out parameters. This also seems to be noticeably faster.Summary by CodeRabbit
Refactor
Style
Bug Fixes
Tests