chore: Sonar fixes after detector refactor #3968
Conversation
WalkthroughChanges made, significant they are. In 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
Component - Core
paulgessinger
changed the title
paulgessinger
force-pushed
the
chore/sonar-post-detector-refactor
branch
from
ed3e144 to
cbf8f78
Compare
github-actions
bot
removed
Component - Core
![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)
Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp (1)
121-136: Duplicate sorting logic, I see. Refactor this, we should.Similar lambda functions, four times repeated they are. A template function or common comparator, better it would be.
+ auto compareDetElements = [](const dd4hep::DetElement& a, + const dd4hep::DetElement& b) { + return (a.id() < b.id()); + }; - std::ranges::sort( - muon, [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); + std::ranges::sort(muon, compareDetElements); - std::ranges::sort( - eCal, [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); + std::ranges::sort(eCal, compareDetElements); // Apply similar changes to hCal and tracker sorting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/Geant4Simulation.hpp(1 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignedDetector.hpp(1 hunks)Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp(3 hunks)
🔇 Additional comments (3)
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignedDetector.hpp (1)
25-29: Careful with platform compatibility, you must be.
Changed from std::size_t to unsigned int, these configuration parameters have. On 64-bit systems where large values you need, limitations this may bring. Consider these implications, you should:
- On 64-bit systems,
std::size_tis typically 64 bits unsigned intusually 32 bits remains, regardless of platform
Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp (1)
89-90: Wisdom in const-correctness, I sense.
The Force is strong with this change. Using const auto& prevents accidental modifications and improves clarity, it does.
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/Geant4Simulation.hpp (1)
211-211: Changed from final to override, the execute method has. Implications, this brings.
Removed the final specifier, more flexibility it provides. But careful consideration needed:
- Derived classes may now override this method
- Ensure documentation reflects this design change
- Verify all derived classes maintain the contract
|
Blocked by:
Summary by CodeRabbit
New Features
DD4hepDetectorclass by using const references in loop iterations.Bug Fixes
Configstruct of theAlignedDetectorclass for better performance and accuracy.Configstruct in theAlignedDetectorclass.