Repository navigation
[PWGUD] Fix casts - #18392
Open
vkucera wants to merge 1 commit into
Open
[PWGUD] Fix casts#18392vkucera wants to merge 1 commit into
vkucera wants to merge 1 commit into
Conversation
|
O2 linter results: ❌ 0 errors, |
Collaborator
|
Error while checking build/O2Physics/code-check for f969d1d at 2026-10-22 01:14: Full log here. |
Collaborator
vkucera
marked this pull request as draft
October 21, 2026 23:27
rolavick
approved these changes
Oct 22, 2026
| auto alppar = ccdb->getForTimeStamp<o2::itsmft::DPLAlpideParam<0>>("ITS/Config/AlpideParam", sor); | ||
| rofShift = alppar->roFrameBiasInBC; | ||
| rofLength = alppar->roFrameLengthInBC; | ||
| rofPerOrbit = static_cast<int>(o2::constants::lhc::LHCMaxBunches / rofLength); |
Collaborator
There was a problem hiding this comment.
imo the static_cast is not indeed needed here, but it is ill-coded anyway, because the result is not rounded, but truncated. We do not know the intentions of the author, maybe it is on purpose like that, so I would judge this as ok fix.
Collaborator
There was a problem hiding this comment.
Just to add, in principle the truncation should not matter as one expects the LHCMaxBunches to be an integral multiple of ROF length. However, the ROF lenght is taken from a configuration file and hence it depends on human who writes the value there. A guard is missing.
Collaborator
Author
There was a problem hiding this comment.
What do you mean? rofLength is int regardless of what alppar->roFrameLengthInBC returns.
Collaborator
There was a problem hiding this comment.
Yes, rofLength is always int, but the value is taken from a calibration file and can be, in principle, any integer. You have LHCMaxBunches = 3564 bunches and for PbPb we always had ROF of length 594. Then, your rofPerOrbit is exactly 6, which is what you want to learn here. But if someone makes a typo in the calibration file and write for example 595, then this code would return rofPerOrbit is 5. That does not look suspicious. The mistake would then propagate further, while not being easily trackable.
Collaborator
Author
There was a problem hiding this comment.
Ah OK, you meant that the task should have a sanity check verifying that LHCMaxBunches is divisible by rofLength. Thanks for the clarification.
Collaborator
|
Well, I think it is ok, but I recommend to make one test run of SGProducer to see what effect has the change of cast there. If I understand it correctly, the previous was potentially dangerous (but it does not necessarily mean mistake happened) Roman |
vkucera
marked this pull request as ready for review
October 22, 2026 12:49
Collaborator
Collaborator
Collaborator
Fixes C-style, redundant, wrong, and dangerous casts (found by Clang-Tidy and cpplint). - Remove redundant casts. (`readability-redundant-casting`) - Remove `const_cast`. (`cppcoreguidelines-pro-type-const-cast`) - Replace C-style casts with `static_cast`. (`google-readability-casting`, cpplint: `readability/casting`) - Replace unsafe C-style casts with `dynamic_cast`. (`cppcoreguidelines-pro-type-cstyle-cast`) - Replace `static_cast` in downcasts with `dynamic_cast`. (`cppcoreguidelines-pro-type-static-cast-downcast`) - Replace `reinterpret_cast` with `static_cast` or `dynamic_cast`. (`cppcoreguidelines-pro-type-reinterpret-cast`) - Replace two-step casting through `void*` with `reinterpret_cast`. (`bugprone-casting-through-void`)
Collaborator
|
Error while checking build/O2Physics/code-check for e95cbf6 at 2026-10-22 19:17: Full log here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes C-style, redundant, wrong, and dangerous casts (found by Clang-Tidy and cpplint).
readability-redundant-casting)const_cast. (cppcoreguidelines-pro-type-const-cast)static_cast. (google-readability-casting, cpplint:readability/casting)dynamic_cast. (cppcoreguidelines-pro-type-cstyle-cast)static_castin downcasts withdynamic_cast. (cppcoreguidelines-pro-type-static-cast-downcast)reinterpret_castwithstatic_castordynamic_cast. (cppcoreguidelines-pro-type-reinterpret-cast)void*withreinterpret_cast. (bugprone-casting-through-void)