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 || operator usage in conditional statements #19231

Merged
merged 1 commit into from Jun 22, 2017

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Jun 15, 2017

This resolves 4 warnings from GCC 7.1.1 where we most likely have a
wrong usage of || operator in our conditional statements.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

This resolves 4 warnings from GCC 7.1.1 where we most likely have a
wrong usage of || operator in our conditional statements.

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@cmsbuild
Copy link
Contributor

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

It involves the following packages:

CalibTracker/SiPixelConnectivity
CalibTracker/SiStripChannelGain

@ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @gbenelli, @tocheng, @jlagram, @dkotlins, @OlivierBondu, @mmusich this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@davidlt
Copy link
Contributor Author

davidlt commented Jun 16, 2017

please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@arunhep
Copy link
Contributor

arunhep commented Jun 20, 2017

@davidlt tests-started label is there for 4 days. I see this in many other PRs. In those, I aborted the tests and started again. I hope this is the right thing to do.

@arunhep
Copy link
Contributor

arunhep commented Jun 21, 2017

@cmsbuild please abort

@cmsbuild
Copy link
Contributor

Jenkins tests are aborted.

@arunhep
Copy link
Contributor

arunhep commented Jun 21, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 21, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20806/console Started: 2017/06/21 10:09

@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-19231/20806/summary.html

Comparison Summary:

  • You potentially added 12 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1803229
  • DQMHistoTests: Total failures: 96
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1802967
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@arunhep
Copy link
Contributor

arunhep commented Jun 22, 2017

+1

@cmsbuild
Copy link
Contributor

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@@ -684,7 +684,7 @@ void SiStripGainFromCalibTree::algoBeginJob(const edm::EventSetup& iSetup)
for(unsigned int i=0;i<Det.size();i++){ //Make two loop such that the Pixel information is added at the end --> make transition simpler
DetId Detid = Det[i]->geographicalId();
int SubDet = Detid.subdetId();
if( SubDet == PixelSubdetector::PixelBarrel || PixelSubdetector::PixelEndcap ){
if( SubDet == PixelSubdetector::PixelBarrel || SubDet == PixelSubdetector::PixelEndcap ){
Copy link
Contributor

Choose a reason for hiding this comment

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

regarding this particular line of the fix, together with the other two similar occurrences in the other classes in the same package, this actually alters the behavior of the code but thanks to the protection few lines below: https://github.com/cms-sw/cmssw/pull/19231/files#diff-5b549e5390591af6a4ae6ece2166765bR689, there is no issue with previous versions of the calibrations.

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b5f8a79 into cms-sw:master Jun 22, 2017
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

5 participants