-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DQM] [Clang]Cleanup clang-analyzer warnings #46227
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46227/42038
|
A new Pull Request was created by @smuzaffar for master. It involves the following packages:
@antoniovagnerini, @cmsbuild, @nothingface0, @rvenditti, @syuvivida, @tjavaid can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@@ -163,6 +166,7 @@ int CaloTowersClient::CaloTowersEndjob(const std::vector<MonitorElement*>& hcalM | |||
|
|||
cnorm = sumphi / phi_factor; | |||
enorm = sqrt(sumphie) / phi_factor; | |||
assert(occupancy_vs_ieta); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cms-sw/dqm-l2 , may be there is better way to fix this code . All these pointers are not set in https://github.com/cms-sw/cmssw/blob/master/Validation/CaloTowers/src/CaloTowersClient.cc#L63-L87 block. So clang-analyzer warns that these might be null. For now I have added assert
to instruct clang static analyzer that we fail if these are null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your proposed solution for the null pointer error handling is fine.
@@ -685,6 +686,7 @@ void DTRecHitQuality::compute(const DTGeometry &dtGeom, | |||
} | |||
} | |||
// Fill | |||
assert(hEff); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, just to make sure these pointers are not null
@@ -144,6 +144,7 @@ void EnergyScaleAnalyzer::analyze(const edm::Event &evt, const edm::EventSetup & | |||
Labels l; | |||
labelsForToken(hepMCLabel_, l); | |||
|
|||
[[clang::suppress]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
genEvent
is used later in the code. Clang static analyzer complains that if hepMC.isValid()
is not valid then genEvent
is not used. [[clang::suppress]]
here instruct clang analyzer to ignore this warnings
There is one warning [a] in https://github.com/cms-sw/cmssw/blob/master/Validation/EcalDigis/plugins/EcalMixingModuleValidation.cc#L47 which I have not fixed. looks like we force set [a]
|
@@ -741,7 +741,7 @@ void GlobalDigisAnalyzer::fillECal(const edm::Event &iEvent, const edm::EventSet | |||
bool validDigiES = true; | |||
if (!EcalDigiES.isValid()) { | |||
LogDebug(MsgLoggerCat) << "Unable to find EcalDigiES in event!"; | |||
validDigiES = false; | |||
[[clang::suppress]] validDigiES = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang analyzer complains as we force set validDigiES = false;
at few line later
@@ -1468,7 +1468,7 @@ void GlobalDigisAnalyzer::fillMuon(const edm::Event &iEvent, const edm::EventSet | |||
bool validrpcdigi = true; | |||
if (!rpcDigis.isValid()) { | |||
LogDebug(MsgLoggerCat) << "Unable to find rpcDigis in event!"; | |||
validrpcdigi = false; | |||
[[clang::suppress]] validrpcdigi = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang analyzer complains as we force set validrpcdigi = false;
at few line later
please test |
+1 Size: This PR adds an extra 88KB to repository Comparison SummarySummary:
|
+1
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @rappoccio, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
This PR fixes clang-analyzer warnings https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_14_2_X_2024-10-02-1100/el8_amd64_gcc12/build-logs/ .