chore: Add google-explicit-constructor to clang-tidy config
#3972
Conversation
WalkthroughAdditions made to Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
github-actions
bot
added
the
Infrastructure
![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
🧹 Outside diff range and nitpick comments (1)
cmake/ActsStaticAnalysis.cmake (1)
18-18: Words of wisdom, share I must!Help developers understand the way of explicit constructors, we should. Consider these actions, you must:
- Document in coding guidelines, the requirement for explicit constructors
- Share examples of proper usage, wisdom brings
- Prepare the team for CI feedback, patience they will need
Strong in the ways of static analysis, this project becomes!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.clang-tidy(1 hunks)cmake/ActsStaticAnalysis.cmake(1 hunks)
🔇 Additional comments (2)
.clang-tidy (1)
7-7: Hmmmm, good this addition is!
Properly placed among the Google checks, the explicit constructor check is. Prevent accidental implicit conversions, it will. A wise decision, this is!
Run this script to check impact on codebase, we must:
✅ Verification successful
The Force guides us to a more specific search, it does! Regular expressions, we shall use, as AST parsing proved challenging, hmm.
Hmm, constructors in the codebase, I have found!
Many C++ files with constructors, there are. But explicit constructors already in use throughout the codebase, I see. A good practice this is, preventing accidental conversions it does. The addition of google-explicit-constructor check, wise it is!
Examples of existing explicit constructors in the codebase, I found:
- In
Acts/Utilities/Logger.hpp - In
Acts/Material/Material.hpp - In
Acts/EventData/SourceLink.hpp - In many other core components
The Force is strong with this change. Maintain consistency with existing codebase practices, it will.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential implicit constructors that might be affected
# Look for single-parameter constructors that might need explicit keyword
ast-grep --pattern 'class $_ {
$$$
$_($_) {
$$$
}
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Search for potential constructors in C++ files
fd -e cpp -e h -e hpp --exec rg -l "^\s*[A-Za-z_][A-Za-z0-9_]*\s*\([^)]+\)\s*(?::\s*\w+\s*\([^)]*\)\s*)*\s*\{" {}
# Also search for explicit keyword usage to understand current practices
fd -e cpp -e h -e hpp --exec rg -l "explicit\s+[A-Za-z_][A-Za-z0-9_]*\s*\([^)]+\)" {}
Length of output: 65397
cmake/ActsStaticAnalysis.cmake (1)
18-18: In harmony with .clang-tidy, this change is!
Consistent with the force of .clang-tidy configuration, this addition remains. Properly appended to the list of checks, it is. Balance to the static analysis, it brings!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmake/ActsStaticAnalysis.cmake (1)
Line range hint
1-50: Additional wisdom to share, I have!Consider documenting the purpose of clang-tidy checks in comments, we should. Help future Padawans understand our ways, it will.
Add this comment before the check configuration:
if(ACTS_RUN_CLANG_TIDY) + # Configure clang-tidy checks + # - google-explicit-constructor: Warns on implicit conversions through constructors + # Add similar documentation for other checks find_program(CLANG_TIDY_COMMAND NAMES clang-tidy)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.clang-tidy(1 hunks)cmake/ActsStaticAnalysis.cmake(1 hunks)
🔇 Additional comments (2)
.clang-tidy (1)
7-7: Approve this change, I do! Wisdom in explicit constructors, there is.
Correctly placed and formatted, the new check is. Prevent accidental implicit conversions, it will. Documentation for this check, we shall verify.
✅ Verification successful
The previous search only showed configuration files. Let's search for actual code patterns and documentation to better understand the impact of this check.
Approve this change with confidence, I do! Strong with the Force, it is!
Evidence in the codebase, we have found! Many explicit constructors already in use, there are. A established pattern in the project, this is. The google-explicit-constructor check, enforce these good practices throughout the codebase, it will.
Examples of wisdom in the codebase, we see:
- In
Acts/Material/HomogeneousVolumeMaterial.hpp, explicit constructors already used, they are - In
Acts/Plugins/Detray/DetrayConversionUtils.hpp, explicit constructor pattern followed, it is - In
Acts/Geometry/Volume.hpp, explicit constructors documented and implemented, they are
Prevent accidental implicit conversions and make code intentions clearer, this check will. Aligned with existing codebase practices, it is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the documentation and implementation of google-explicit-constructor check
# Search for documentation or implementation details of the check
rg -A 5 "google-explicit-constructor"
Length of output: 844
Script:
#!/bin/bash
# Search for any files that might be affected by the google-explicit-constructor check
# Look for implicit constructors that might be flagged
ast-grep --pattern 'class $_ {
$_($$$) {
$$$
}
}'
# Also search for any test files or examples related to this check
rg -i "implicit.*constructor|explicit.*constructor" -A 3
Length of output: 4961
cmake/ActsStaticAnalysis.cmake (1)
18-18: Consistent with .clang-tidy, this change is. Approve it, I do!
In harmony with the Force, both configuration files now are. The same check in both places, we see.
✅ Verification successful
Perfect alignment between files, I sense. Approve this change with confidence, we must!
Examined the Force flows between .clang-tidy and cmake/ActsStaticAnalysis.cmake, I have. In perfect harmony, they are:
- The same checks in both files, present they are
- In identical order, they flow
- The
google-explicit-constructorcheck, properly placed it is
Strong with the Force, this change is. No disturbance in the configuration files, I detect.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency between .clang-tidy and CMake configuration
# Compare the checks in both files
echo "Checking consistency between configurations..."
rg -A 20 "Checks: " .clang-tidy
rg -A 20 "list\(APPEND _chks" cmake/ActsStaticAnalysis.cmake
Length of output: 2920
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CI/clang_tidy/limits.yml (1)
Line range hint
1-24: Document the meaning of zero, we should.Hmmmm... Future maintainers benefit from clarity, they will. Add comment explaining that zero means "no limit on violations", we should.
Add this comment at the top of the file, you should:
+# Limits configuration for clang-tidy checks +# A value of 0 means no limit on the number of violations reported + limits: "clang-analyzer-optin.cplusplus.UninitializedObject": 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CI/clang_tidy/limits.yml(1 hunks)
🔇 Additional comments (1)
CI/clang_tidy/limits.yml (1)
7-7: Approve this change, I do. But verify the full implementation, we must.
Correctly placed and configured, the new check is. But ensure that enabled in .clang-tidy file, it also is.
Run this verification script, we shall:
✅ Verification successful
Verified the implementation is, young padawan.
Found in both places, the check is:
- In
.clang-tidy, enabled as part of checks list, it is - In
cmake/ActsStaticAnalysis.cmake, properly configured, it appears
Complete and correct, this implementation is. Working together in harmony, all components are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete implementation of google-explicit-constructor
# Check if the check is enabled in .clang-tidy
echo "Search for google-explicit-constructor in .clang-tidy, we must:"
rg "google-explicit-constructor" .clang-tidy
# Check if CMake configuration includes this check
echo "Search for google-explicit-constructor in CMake files, we shall:"
rg "google-explicit-constructor" cmake/
Length of output: 481
Needs a few prerequisite changes:
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Summary by CodeRabbit
google-explicit-constructorto enhance code quality.