Fix gcc 9 warnings in DataFormats #27844

mrodozov · 2019-08-23T14:32:33Z

PR description:

Fixes warnings in DataFormats gcc9 IB

PR validation:

Builds without warnings. Some warnings were from other packages (PR still open)

mrodozov · 2019-08-23T14:32:42Z

please test

cmsbuild · 2019-08-23T14:33:04Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-08-23T14:39:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27844/11581

This PR adds an extra 24KB to repository

cmsbuild · 2019-08-23T14:40:13Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2146/console Started: 2019/08/23 17:17

cmsbuild · 2019-08-23T14:40:15Z

A new Pull Request was created by @mrodozov (Mircho Rodozov) for master.

It involves the following packages:

DataFormats/EgammaCandidates
DataFormats/HcalCalibObjects
DataFormats/TrackReco
DataFormats/TrackerCommon

@perrotta, @slava77, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @pohsun can you please review it and eventually sign? Thanks.
@Sam-Harper, @makortel, @jainshilpi, @tocheng, @VinInn, @rovere, @gpetruc, @mmusich, @lgray, @varuns23 this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

perrotta · 2019-08-23T16:21:45Z

DataFormats/HcalCalibObjects/src/HFRecalibration.cc

@@ -35,7 +35,7 @@ double HFRecalibration::getCorr(int ieta, int depth, double lumi) {
  switch (depth) {
    case 1:
      reCalFactor = (1 + HFParsAB[0][0][ieta] * sqrt(lumi) + HFParsAB[0][1][ieta] * lumi);
-
+      [[fallthrough]];


I suspect this wasn't originally supposed to fall through: @abdoulline @igv4321 ?

@perrotta
I suppose there had to be "break" originally (apparently forgotten by author), as HF depths are treated separately (method parameter list has "depth" parameter).
So fall though doesn't seem to be natural here...

mmusich · 2019-08-23T16:25:59Z

DataFormats/TrackerCommon/src/TrackerTopology.cc

@@ -396,6 +396,7 @@ SiStripModuleGeometry TrackerTopology::moduleGeometry(const DetId &id) const {
        case 3:
          return SiStripModuleGeometry::W3A;
      }
+      [[fallthrough]];


I think this should never fall through in real life, though, I would rather break here, after returning an SiStripModuleGeometry::UNKNOWNGEOMETRY;

cmsbuild · 2019-08-23T17:09:01Z

+1
Tested at: 31a2d85
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-885847/2146/summary.html

cmsbuild · 2019-08-23T17:09:05Z

Comparison job queued.

cmsbuild · 2019-08-23T19:02:18Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-885847/2146/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2939508
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2939166
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 142 log files, 14 edm output root files, 34 DQM output files

perrotta · 2019-08-24T09:50:27Z

DataFormats/EgammaCandidates/src/Photon.cc

@@ -90,6 +90,7 @@ void Photon::setCorrectedEnergy(P4type type, float newEnergy, float delta_e, boo
    case regression1:
      eCorrections_.regression1Energy = newEnergy;
      eCorrections_.regression1EnergyError = delta_e;
+      [[fallthrough]];


Shouldn't it rather break here? @Sam-Harper

yes, this is bug, thanks for pointing this out. Luckily I dont think we use regression1 and its regression2 that is our main correction. Likewise for comment below

perrotta · 2019-08-24T09:50:33Z

DataFormats/EgammaCandidates/src/Photon.cc

@@ -150,6 +151,7 @@ void Photon::setP4(P4type type, const LorentzVector& p4, float error, bool setTo
    case regression1:
      eCorrections_.regression1P4 = p4;
      eCorrections_.regression1EnergyError = error;
+      [[fallthrough]];


Shouldn't it rather break here? @Sam-Harper

yes, see above comment, thanks for spotting!

perrotta · 2019-08-24T09:53:41Z

Once could profit and further remove unneeded break statements in Photon::getCorrectedEnergy and Photon::getCorrectedEnergyError

perrotta · 2019-08-26T14:02:37Z

@mrodozov , all updates have been addressed separately, and it looks like that the correct fix is here in all cases to replace the fallthrough with an apparently forgotten break statement.
Will you take care of updating this PR accordingly? Or should I do it?

cmsbuild · 2019-09-03T09:40:49Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-09-03T09:48:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27844/11741

This PR adds an extra 28KB to repository

cmsbuild · 2019-09-03T09:48:39Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2326/console Started: 2019/09/03 11:49

cmsbuild · 2019-09-03T09:48:40Z

Pull request #27844 was updated. @perrotta, @slava77, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @pohsun can you please check and sign again.

cmsbuild · 2019-09-03T12:27:17Z

+1
Tested at: 82e24bc
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-885847/2326/summary.html

cmsbuild · 2019-09-03T12:27:21Z

Comparison job queued.

cmsbuild · 2019-09-03T14:50:19Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-885847/2326/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2955700
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2955358
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 145 log files, 15 edm output root files, 34 DQM output files

perrotta · 2019-09-04T05:50:51Z

+1

[[fallthrough]] directive inserted in DataFormats/EgammaCandidates/src/Photon.cc, to replicate the identical behaviour as before
fallthrough replaced by a break in DataFormats/HcalCalibObjects/src/HFRecalibration.cc (bug fix): this could affect HF aging scenarios in Phase2
fallthrough replaced by a return of UNKNOWNGEOMETRY in DataFormats/TrackerCommon/src/TrackerTopology.cc (logical bug fix, but quite likely never hit):
Jenkins tests pass and show no difference

pohsun · 2019-09-05T08:14:36Z

+1

cmsbuild · 2019-09-05T08:15:06Z

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-09-06T07:51:51Z

+1

Fix gcc 9 warnings in DataFormats

31a2d85

cmsbuild added this to the CMSSW_11_0_X milestone Aug 23, 2019

cmsbuild added alca-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 23, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 23, 2019

cmsbuild added tests-started and removed tests-pending labels Aug 23, 2019

perrotta reviewed Aug 23, 2019

View reviewed changes

mmusich reviewed Aug 23, 2019

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Aug 23, 2019

cmsbuild added comparison-available and removed comparison-pending labels Aug 23, 2019

perrotta reviewed Aug 24, 2019

View reviewed changes

cmsbuild added comparison-pending tests-pending labels Sep 3, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 3, 2019

cmsbuild added tests-started and removed tests-pending labels Sep 3, 2019

cmsbuild added tests-approved and removed tests-started labels Sep 3, 2019

cmsbuild added comparison-available and removed comparison-pending labels Sep 3, 2019

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 4, 2019

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Sep 5, 2019

cmsbuild added orp-approved and removed orp-pending labels Sep 6, 2019

cmsbuild merged commit 28f9d39 into cms-sw:master Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gcc 9 warnings in DataFormats #27844

Fix gcc 9 warnings in DataFormats #27844

mrodozov commented Aug 23, 2019

mrodozov commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019 •

edited

Loading

cmsbuild commented Aug 23, 2019

perrotta Aug 23, 2019 •

edited

Loading

abdoulline Aug 26, 2019

mmusich Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

perrotta Aug 24, 2019

Sam-Harper Aug 26, 2019

perrotta Aug 24, 2019

Sam-Harper Aug 26, 2019

perrotta commented Aug 24, 2019

perrotta commented Aug 26, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019 •

edited

Loading

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

perrotta commented Sep 4, 2019

pohsun commented Sep 5, 2019

cmsbuild commented Sep 5, 2019

fabiocos commented Sep 6, 2019

Fix gcc 9 warnings in DataFormats #27844

Fix gcc 9 warnings in DataFormats #27844

Conversation

mrodozov commented Aug 23, 2019

PR description:

PR validation:

mrodozov commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019 • edited Loading

cmsbuild commented Aug 23, 2019

perrotta Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

abdoulline Aug 26, 2019

Choose a reason for hiding this comment

mmusich Aug 23, 2019

Choose a reason for hiding this comment

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

cmsbuild commented Aug 23, 2019

perrotta Aug 24, 2019

Choose a reason for hiding this comment

Sam-Harper Aug 26, 2019

Choose a reason for hiding this comment

perrotta Aug 24, 2019

Choose a reason for hiding this comment

Sam-Harper Aug 26, 2019

Choose a reason for hiding this comment

perrotta commented Aug 24, 2019

perrotta commented Aug 26, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019 • edited Loading

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

cmsbuild commented Sep 3, 2019

perrotta commented Sep 4, 2019

pohsun commented Sep 5, 2019

cmsbuild commented Sep 5, 2019

fabiocos commented Sep 6, 2019

cmsbuild commented Aug 23, 2019 •

edited

Loading

perrotta Aug 23, 2019 •

edited

Loading

cmsbuild commented Sep 3, 2019 •

edited

Loading