[RECONSTRUCTION] [Clang]Cleanup clang-analyzer warnings #46258

smuzaffar · 2024-10-04T12:49:41Z

This PR fixes clang-analyzer warnings

cmsbuild · 2024-10-04T12:50:06Z

cms-bot internal usage

cmsbuild · 2024-10-04T12:52:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46258/42081

There are other open Pull requests which might conflict with changes you have proposed:
- File RecoJets/JetProducers/plugins/VirtualJetProducer.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

cmsbuild · 2024-10-04T12:53:05Z

A new Pull Request was created by @smuzaffar for master.

It involves the following packages:

RecoJets/JetPlusTracks (reconstruction)
RecoJets/JetProducers (reconstruction)
RecoMET/METAlgorithms (reconstruction)
RecoTBCalo/EcalTBHodoscopeReconstructor (reconstruction)
RecoTBCalo/HcalTBObjectUnpacker (reconstruction)
RecoTauTag/RecoTau (reconstruction)
RecoTracker/FinalTrackSelectors (reconstruction)
RecoTracker/MkFitCore (reconstruction)
RecoTracker/PixelSeeding (reconstruction)
RecoTracker/TkSeedGenerator (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@AnnikaStein, @GiacomoSguazzoni, @JanFSchulte, @ReyerBand, @VinInn, @VourMa, @ahinzmann, @argiro, @azotz, @bsunanda, @clelange, @dgulhan, @felicepantaleo, @gkasieczka, @gouskos, @gpetruc, @jdamgov, @jdolen, @makortel, @mariadalfonso, @mbluj, @missirol, @mmarionncern, @mmusich, @mtosi, @nhanvtran, @rappoccio, @rchatter, @rovere, @schoef, @seemasharmafnal, @thomreis, @wang0jin, @yslai this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2024-10-04T13:04:29Z

RecoJets/JetPlusTracks/plugins/JetPlusTrackProducer.cc

@@ -195,13 +190,13 @@ void JetPlusTrackProducer::produce(edm::Event& iEvent, const edm::EventSetup& iS
    bool validMatches = false;

    if (!vectorial_) {
-      scaleJPT = mJPTalgo->correction(corrected, oldjet, iEvent, iSetup, pions, muons, elecs, validMatches);
+      double scaleJPT = mJPTalgo->correction(corrected, oldjet, iEvent, iSetup, pions, muons, elecs, validMatches);


looks like scaleJPT was only use in this block of code, so I have moved the definition here

smuzaffar · 2024-10-04T13:04:47Z

RecoJets/JetPlusTracks/plugins/JetPlusTrackProducerAA.cc

@@ -157,13 +153,13 @@ void JetPlusTrackProducerAA::produce(edm::Event& iEvent, const edm::EventSetup&
    bool validMatches = false;

    if (!vectorial_) {
-      scaleJPT = mJPTalgo->correction(corrected, oldjet, iEvent, iSetup, pions, muons, elecs, validMatches);
+      double scaleJPT = mJPTalgo->correction(corrected, oldjet, iEvent, iSetup, pions, muons, elecs, validMatches);


looks like scaleJPT was only use in this block of code, so I have moved the definition here

smuzaffar · 2024-10-04T13:05:17Z

RecoJets/JetProducers/plugins/VirtualJetProducer.cc

@@ -752,6 +752,8 @@ void VirtualJetProducer::writeJets(edm::Event& iEvent, edm::EventSetup const& iS
        // Here it is assumed that fjJets_ is in decreasing order of pT,
        // which should happen in FastjetJetProducer::runAlgorithm()
        jetArea = M_PI;
+        //rij is properly initialized above
+        [[clang::suppress]]


rij is properly initialized above. It is false positive report from cland-analyzer.

smuzaffar · 2024-10-04T13:06:15Z

RecoJets/JetProducers/src/PileupJetIdAlgo.cc

@@ -338,6 +338,7 @@ PileupJetIdentifier PileupJetIdAlgo::computeIdVariables(const reco::Jet* jet,
  for (unsigned i = 0; i < jet->numberOfSourceCandidatePtrs(); ++i) {
    reco::CandidatePtr pfJetConstituent = jet->sourceCandidatePtr(i);
    const reco::Candidate* icand = pfJetConstituent.get();
+    assert(icand);


just to silence the clang-analyzer warning. clang analyzer clains that pfJetConstituent.get() can return nullptr

smuzaffar · 2024-10-04T13:07:45Z

RecoMET/METAlgorithms/src/SignAlgoResolutions.cc

    string cmssw_base(std::getenv("CMSSW_BASE"));
+    [[clang::suppress]]


suppress these warnings. in cmssw env both CMSSW_BASE and CMSSW_RELEASE_BASE are set

I think this code should use edm::FileInPath instead.

smuzaffar · 2024-10-04T13:46:51Z

RecoTracker/TkSeedGenerator/plugins/MultiHitGeneratorFromChi2.cc

@@ -233,7 +233,7 @@ void MultiHitGeneratorFromChi2::hitSets(const TrackingRegion& region,
  foundNodes.reserve(100);
  declareDynArray(KDTreeLinkerAlgo<RecHitsSortedInPhi::HitIter>, nThirdLayers, hitTree);
  declareDynArray(LayerRZPredictions, nThirdLayers, mapPred);
-  float rzError[nThirdLayers];  //save maximum errors
+  std::vector<float> rzError(nThirdLayers);  //save maximum errors


to avoid variable length array

smuzaffar · 2024-10-04T13:47:01Z

RecoTracker/PixelSeeding/plugins/PixelTripletLargeTipGenerator.cc

@@ -169,7 +169,7 @@ void PixelTripletLargeTipGenerator::hitTriplets(const TrackingRegion& region,
  declareDynArray(KDTreeLinkerAlgo<unsigned int>, nThirdLayers, hitTree);
  declareDynArray(LayerRZPredictions, nThirdLayers, mapPred);

-  float rzError[nThirdLayers];  //save maximum errors
+  std::vector<float> rzError(nThirdLayers);  //save maximum errors


to avoid variable length array

smuzaffar · 2024-10-04T13:47:09Z

RecoTracker/PixelSeeding/plugins/PixelTripletHLTGenerator.cc

@@ -133,7 +133,7 @@ void PixelTripletHLTGenerator::hitTriplets(const TrackingRegion& region,
  foundNodes.reserve(100);

  declareDynArray(KDTreeLinkerAlgo<unsigned int>, nThirdLayers, hitTree);
-  float rzError[nThirdLayers];  //save maximum errors
+  std::vector<float> rzError(nThirdLayers);  //save maximum errors


to avoid variable length array

mbluj · 2024-10-04T14:16:58Z

RecoTauTag/RecoTau/src/RecoTauCommonUtilities.cc

@@ -111,6 +111,7 @@ namespace reco {
      if (pfCand)
        return pfCand->positionAtECALEntrance();

+      assert(part);


As far as I understand it it is protection against nullptr, right? It is unlikely to happen, but it is OK to have this protection. But, would it be more logical to move this assert to l.110 before dynamic_cast?

Is it "unlikely to happen" or "should never happen"? If it is the latter, how about changing the function to take the part as const reference instead of pointer to const?

I think both :) I think nobody have touched this code for years, but I think it is called with well behaving, i.e. non null pointers. Anyway I agree that using const ref is better design than what is present currently. I can put this to my todo, butI think that in scope of this PR it makes sense to move the assert a few lines earlier.

sure, I will move it up

cmsbuild · 2024-10-04T15:45:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46258/42091

There are other open Pull requests which might conflict with changes you have proposed:
- File RecoJets/JetProducers/plugins/VirtualJetProducer.cc modified in PR(s): [RFC] descriptions.addDefault -> descriptions.addWithDefaultLabel #44372

cmsbuild · 2024-10-04T15:45:36Z

Pull request #46258 was updated. @cmsbuild, @jfernan2, @mandrenguyen can you please check and sign again.

jfernan2 · 2024-10-07T14:04:48Z

please test

cmsbuild · 2024-10-07T19:26:03Z

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-542d7d/42024/summary.html
COMMIT: 5af2b72
CMSSW: CMSSW_14_2_X_2024-10-07-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46258/42024/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 3 lines from the logs
Reco comparison results: 28 differences found in the comparisons
DQMHistoTests: Total files compared: 45
DQMHistoTests: Total histograms compared: 3438002
DQMHistoTests: Total failures: 1433
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3436549
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 44 files compared)
Checked 197 log files, 168 edm output root files, 45 DQM output files
TriggerResults: no differences found

jfernan2 · 2024-10-08T08:16:59Z

+1

cmsbuild · 2024-10-08T08:17:20Z

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. @rappoccio, @antoniovilela, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

mandrenguyen · 2024-10-08T08:52:48Z

+1

[RECONSTRUCTION] [Clang]Cleanup clang-analyzer warnings

3da51ef

cmsbuild added this to the CMSSW_14_2_X milestone Oct 4, 2024

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending tracking labels Oct 4, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 4, 2024

smuzaffar commented Oct 4, 2024

View reviewed changes

mbluj reviewed Oct 4, 2024

View reviewed changes

code review: use FileInPath

5af2b72

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 4, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 4, 2024

cmsbuild mentioned this pull request Oct 7, 2024

[LLVM Analyzer][Reconstuction] Remove dead code #46281

Closed

cmsbuild added tests-started and removed tests-pending labels Oct 7, 2024

cmsbuild added tests-approved and removed tests-started labels Oct 7, 2024

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Oct 8, 2024

cmsbuild added orp-approved and removed orp-pending labels Oct 8, 2024

cmsbuild merged commit d4aca16 into cms-sw:master Oct 8, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RECONSTRUCTION] [Clang]Cleanup clang-analyzer warnings #46258

[RECONSTRUCTION] [Clang]Cleanup clang-analyzer warnings #46258

smuzaffar commented Oct 4, 2024

cmsbuild commented Oct 4, 2024 •

edited

Loading

cmsbuild commented Oct 4, 2024

cmsbuild commented Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

makortel Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

mbluj Oct 4, 2024

makortel Oct 4, 2024

mbluj Oct 4, 2024

smuzaffar Oct 4, 2024

smuzaffar Oct 4, 2024

cmsbuild commented Oct 4, 2024

cmsbuild commented Oct 4, 2024

jfernan2 commented Oct 7, 2024

cmsbuild commented Oct 7, 2024

jfernan2 commented Oct 8, 2024

cmsbuild commented Oct 8, 2024

mandrenguyen commented Oct 8, 2024

		string cmssw_base(std::getenv("CMSSW_BASE"));
		[[clang::suppress]]

[RECONSTRUCTION] [Clang]Cleanup clang-analyzer warnings #46258

[RECONSTRUCTION] [Clang]Cleanup clang-analyzer warnings #46258

Conversation

smuzaffar commented Oct 4, 2024

cmsbuild commented Oct 4, 2024 • edited Loading

cmsbuild commented Oct 4, 2024

cmsbuild commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Oct 4, 2024

cmsbuild commented Oct 4, 2024

jfernan2 commented Oct 7, 2024

cmsbuild commented Oct 7, 2024

Comparison Summary

jfernan2 commented Oct 8, 2024

cmsbuild commented Oct 8, 2024

mandrenguyen commented Oct 8, 2024

cmsbuild commented Oct 4, 2024 •

edited

Loading