fix: remove narrowing conversion from std::size_t to int
#3932
Conversation
WalkthroughChanges made enhance type safety and clarity in several components of the codebase. The Changes
Possibly related issues
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
![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 (4)
Fatras/include/ActsFatras/Digitization/UncorrelatedHitSmearer.hpp (1)
89-89: Approve this change, I do! Hmmmm.Wise choice it is, to use std::size_t for array indexing. Safer path this leads to, preventing dark side of narrowing conversions it does. Aligned with container's natural size type, this change now is.
Remember young padawan, when dealing with containers in future code:
- Use std::size_t for sizes and indices, you must
- Avoid narrowing conversions to signed types, you shall
- Let the container's natural types guide you, always
Core/src/Detector/detail/IndexedGridFiller.cpp (1)
73-76: Simplify the understep calculation, we can.Correct the logic is, but simpler it could be. A refactor suggestion, I have.
- if (bmin <= expand) { - std::size_t understep = expand - bmin; - fill_linear(nBins - understep, nBins); - } + if (bmin <= expand) { + fill_linear(nBins - (expand - bmin), nBins); + }Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp (1)
67-68: A pattern in the Force, I sense.Consistent with TrackFittingAlgorithm.cpp, this change is. Type safety through static_cast to std::size_t, wisdom brings.
Consider a common utility function for track picking logic, share this pattern across algorithms we should.
Examples/Algorithms/TrackFitting/src/TrackFittingAlgorithm.cpp (1)
106-107: Consistent with RefittingAlgorithm.cpp, this change is.The same pattern of type safety improvement, I observe. Yet, duplicate this logic across files, we should not.
Extract this common pattern into a utility function, we should:
// In a common header file inline bool shouldProcessTrack(std::int64_t pickTrack, std::size_t currentTrack) { return pickTrack <= -1 || static_cast<std::size_t>(pickTrack) == currentTrack; }Then use it in both algorithms:
- if (m_cfg.pickTrack > -1 && - static_cast<std::size_t>(m_cfg.pickTrack) != itrack) { + if (!shouldProcessTrack(m_cfg.pickTrack, itrack)) { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Core/src/Detector/detail/IndexedGridFiller.cpp(1 hunks)Examples/Algorithms/Digitization/include/ActsExamples/Digitization/DigitizationAlgorithm.hpp(2 hunks)Examples/Algorithms/TrackFitting/src/RefittingAlgorithm.cpp(1 hunks)Examples/Algorithms/TrackFitting/src/TrackFittingAlgorithm.cpp(1 hunks)Fatras/include/ActsFatras/Digitization/UncorrelatedHitSmearer.hpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Examples/Algorithms/Digitization/include/ActsExamples/Digitization/DigitizationAlgorithm.hpp
🔇 Additional comments (3)
Fatras/include/ActsFatras/Digitization/UncorrelatedHitSmearer.hpp (1)
Line range hint 89-101: Strong with the Force, this implementation is!
Robust error handling observe I do:
- Gracefully propagate errors through Result type, it does
- Maintain existing safety checks, it continues to do
- Clear and maintainable, the code remains
A balanced approach to type safety and error handling, achieved you have.
Core/src/Detector/detail/IndexedGridFiller.cpp (2)
50-54: Improved type safety in boundary calculations, hmmmm.
Removed narrowing conversions, safer the code now is. Direct comparison with std::size_t, cleaner and more explicit it becomes.
61-61: Verify the closed case boundary condition, we must.
Changed the condition has been, from static_cast to direct comparison. Ensure correct behavior across boundary cases, we should.
✅ Verification successful
Correct and safe, the boundary condition is, hmmmm.
Found the implementation context I have, and clear it is that the condition handles boundary cases with wisdom:
- Checks for sufficient bin space it does (2 * span < nBins)
- Guards against overflow it does (bmax + expand <= nBins)
- Protects against underflow it does (bmin > expand)
- Falls back to safe linear filling when conditions not met, it does
The Force is strong with this code change, as direct size_t comparison more natural it is than casting to int. Maintain the same logical safeguards it does, while cleaner it becomes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering the closed case boundary condition
rg -l "AxisBoundaryType::Closed.*binSequence" --type cpp
Length of output: 58
Script:
#!/bin/bash
# Search for tests related to IndexedGridFiller and boundary conditions
rg -l "IndexedGridFiller.*test" --type cpp
# Search for any test files containing "Closed" and "Grid"
rg -l "Closed.*Grid.*test|Grid.*Closed.*test" --type cpp
# Look for the actual implementation of closed case handling
ast-grep --pattern 'if ($$$span$$$nBins$$$) {
$$$
}' Core/src/Detector/detail/IndexedGridFiller.cpp -A 10
Length of output: 213
Script:
#!/bin/bash
# Search for test files in Tests directory
fd "test.*\.cpp$" Tests/
# Look for the implementation context
rg -B 5 -A 10 "2 \* span < nBins" Core/src/Detector/detail/IndexedGridFiller.cpp
# Find any usage of IndexedGridFiller
rg -l "IndexedGridFiller" --type cpp
Length of output: 22231
|
I went through the cases, where we did
cast<int>fromstd::size_tand implemented solutions to avoid this.closes:
Summary by CodeRabbit
Bug Fixes
inttostd::size_t.Documentation
DigitizationAlgorithmclass.Chores
IndexedGridFillerandRefittingAlgorithmclasses for better readability and maintenance.