Skip to content
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

Fix gcc9 warnings L1Trigger/GlobalCaloTrigger #27827

Merged

Conversation

mrodozov
Copy link
Contributor

PR description:

Fix warnings in L1Trigger/GlobalCaloTrigger

PR validation:

Builds without warnings. Added [[fallthrough]] for every "missing" break.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27827/11551

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

L1Trigger/GlobalCaloTrigger

@cmsbuild, @rekovic, @benkrikler can you please review it and eventually sign? Thanks.
@Martin-Grunewald 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

@mrodozov
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 22, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2123/console Started: 2019/08/22 20:12

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dbb5fb/2123/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: 4
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2939163
  • 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

@@ -305,6 +305,7 @@ void convertToGct(EmInputCandVec candidates) {
} else {
eta = 5;
}
[[fallthrough]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrodozov why [[fallthrough]] instead of break as in all the other cases? I would naively imagine it was just forgotten, looking at the code.
@rekovic could you please check?

@@ -320,6 +321,7 @@ void convertToGct(EmInputCandVec candidates) {
} else {
eta = 3;
}
[[fallthrough]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because with the current state the logic is to fallthrough and I was adding it where it was falling anyway. I was assuming breaks were forgotten, too, and then I've been told not to assume since the fallthrough were intentional. Some of them weren't but most were. Where they are forgotten I've added them like in here:
#27856
but in other places I've been told otherwise:
#27811 (comment)
Solution was to keep the existing logic until told to change it to break by responsible's.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrodozov thanks, I see the reasoning behind, but I would like nevertheless to ask @rekovic to check and confirm that this is the intended behaviour of the code

@fabiocos
Copy link
Contributor

fabiocos commented Sep 8, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented Sep 8, 2019

merge

@cmsbuild cmsbuild merged commit b39fd7f into cms-sw:master Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants