RecoTracker/MeasurementDet : bug fix for gcc 6.0 misleading-indentation warning #15003

gartung · 2016-06-27T21:45:06Z

/build/cmsbuild/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/8258e37281729e179b5af9c3b033d930/opt/cmssw/slc6_amd64_gcc600/cms/cmssw/CMSSW_8_1_X_2016-06-26-2300/src/RecoTracker/MeasurementDet/plugins/TkStripMeasurementDet.cc:83:7: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
for (auto && h: tmp) result.push_back(new SiStripRecHit2D(std::move(h))); tmp.clear();
^~~
/build/cmsbuild/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/8258e37281729e179b5af9c3b033d930/opt/cmssw/slc6_amd64_gcc600/cms/cmssw/CMSSW_8_1_X_2016-06-26-2300/src/RecoTracker/MeasurementDet/plugins/TkStripMeasurementDet.cc:83:81: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'
for (auto && h: tmp) result.push_back(new SiStripRecHit2D(std::move(h))); tmp.clear();
^~~
/build/cmsbuild/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/8258e37281729e179b5af9c3b033d930/opt/cmssw/slc6_amd64_gcc600/cms/cmssw/CMSSW_8_1_X_2016-06-26-2300/src/RecoTracker/MeasurementDet/plugins/TkStripMeasurementDet.cc:90:5: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
for (auto && h: tmp) result.push_back(new SiStripRecHit2D(std::move(h))); tmp.clear();
^~~
/build/cmsbuild/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/8258e37281729e179b5af9c3b033d930/opt/cmssw/slc6_amd64_gcc600/cms/cmssw/CMSSW_8_1_X_2016-06-26-2300/src/RecoTracker/MeasurementDet/plugins/TkStripMeasurementDet.cc:90:79: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'
for (auto && h: tmp) result.push_back(new SiStripRecHit2D(std::move(h))); tmp.clear();
^~~

/build/cmsbuild/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/8258e37281729e179b5af9c3b033d930/opt/cmssw/slc6_amd64_gcc600/cms/cmssw/CMSSW_8_1_X_2016-06-26-2300/src/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.cc:29:4: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (bad128Strip_[j+offset] == false) hasAny128StripBad_[i] = false; break;
^~
/build/cmsbuild/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/8258e37281729e179b5af9c3b033d930/opt/cmssw/slc6_amd64_gcc600/cms/cmssw/CMSSW_8_1_X_2016-06-26-2300/src/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.cc:29:72: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
if (bad128Strip_[j+offset] == false) hasAny128StripBad_[i] = false; break;
^~~~~

cmsbuild · 2016-06-27T21:45:26Z

A new Pull Request was created by @gartung (Patrick Gartung) for CMSSW_8_1_X.

It involves the following packages:

RecoTracker/MeasurementDet

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @mschrode, @gpetruc, @dgulhan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

slava77 · 2016-06-27T22:20:28Z

@cmsbuild please test

cmsbuild · 2016-06-27T22:20:45Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13706/console

cmsbuild · 2016-06-27T23:40:36Z

+1
Tested at: 18d0ab0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15003/13706/summary.html

VinInn · 2016-06-28T06:44:34Z

since when line breaking and indentation have a meaning in C(++)????

cmsbuild · 2016-06-28T07:28:35Z

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

gartung · 2016-06-28T12:25:50Z

Did the author mean to enclose the line in curly braces?

VinInn · 2016-06-28T12:26:50Z

of course not!

gartung · 2016-06-28T12:28:25Z

I don't know that. The warning is to catch the case where the author meant to enclose in curly braces.

gartung · 2016-06-28T12:38:02Z

@Dr15Jones care to comment.

Dr15Jones · 2016-06-28T12:42:36Z

RecoTracker/MeasurementDet/src/TkMeasurementDetSet.cc

+	     hasAny128StripBad_[i] = true;
+	     for (int j = 0; i < (totalStrips_[j] >> 7); j++) {
+	       if (bad128Strip_[j+offset] == false) hasAny128StripBad_[i] = false;
+          break;


This doesn't actually make sense, although matches what the previous version was doing. This code always stops after the first iteration.

Better fix the bug and not loose the warning that points to a problem.
It may make more sense if someone (@VinInn ?) take over.
In the case the bug is fixed, the title of the PR should be changed.

Dr15Jones · 2016-06-28T12:44:34Z

@VinInn the gcc 6 compiler prints warnings when one doesn't use braces for an if and then add additional lines after the first semi-colon. These types of lines were found to often be a source of bugs. In fact, it looks like this actually caught a bug in this code.

Dr15Jones · 2016-06-28T12:45:30Z

@slava77 How does this bug (i.e. the break not actually being in the if) affect reconstruction?

gartung · 2016-06-28T12:45:50Z

@Dr15Jones I started adding curly braces where I thought they should go but then changed to just adding line breaks. In some cases I can't tell what the author meant.

Dr15Jones · 2016-06-28T12:49:28Z

@VinInn Definitely the tmp.clear() should not be in the same block as the for loop. But why is there a break in a for loop if that loop always calls the break? If that is the intended behavior shouldn't the loop just be removed?

VinInn · 2016-06-28T12:50:03Z

@Dr15Jones ,
comment says, // this should not happen, as usually you turn on all fibers
so most probably is never run

VinInn · 2016-06-28T12:51:31Z

the compiler most probably remove the loop!
IN any case I think it is a bug, curly braces should be added there...
(the code is oldish)

VinInn · 2016-06-28T15:26:00Z

@cmsbuild , please test

cmsbuild · 2016-06-28T15:26:21Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13737/console

cmsbuild · 2016-06-28T16:34:17Z

+1
Tested at: 95336ed
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15003/13729/summary.html

cmsbuild · 2016-06-28T17:29:37Z

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

cmsbuild · 2016-06-28T18:36:54Z

+1
Tested at: 95336ed
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15003/13737/summary.html

cmsbuild · 2016-06-28T19:39:31Z

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

cvuosalo · 2016-07-01T21:10:40Z

@gartung: I don't understand your latest change to the PR title. This PR potentially changes run-time behavior in TkMeasurementDetSet.cc by moving the break under the if. The title should mention this bug fix.

gartung · 2016-07-01T21:30:31Z

Sorry. Pasted the wrong title change.

cvuosalo · 2016-07-06T23:00:17Z

+1

For #15003 95336ed

Cleaning up confusing indentation in two TkStripMeasurementDet files. One change is actually a bug fix, but it is in a section of code that was probably never actually executed, so the fix shouldn't change normal run-time behavior. There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-06-28-1100 show no significant differences, as expected.

cmsbuild · 2016-07-06T23:00:36Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

Fix this 'if' clause does not guard... [-Wmisleading-indentation]

18d0ab0

cmsbuild added this to the Next CMSSW_8_1_X milestone Jun 27, 2016

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Jun 27, 2016

cmsbuild added tests-started and removed tests-pending labels Jun 27, 2016

cmsbuild added tests-approved and removed tests-started labels Jun 27, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jun 28, 2016

Dr15Jones reviewed Jun 28, 2016
View reviewed changes

gartung changed the title ~~Fix this 'if' clause does not guard... [-Wmisleading-indentation]~~ gcc 6.0 misleading-indentation warning flags potential bug Jun 28, 2016

cmsbuild added tests-started and removed tests-pending labels Jun 28, 2016

gartung changed the title ~~gcc 6.0 misleading-indentation warning flags potential bug~~ RecoTracker/MeasurementDet : gcc 6.0 misleading-indentation warning flags potential bug; with formatting and bug fix Jun 28, 2016

cmsbuild added tests-approved and removed tests-started labels Jun 28, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jun 28, 2016

cmsbuild added comparison-pending and removed comparison-available labels Jun 28, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jun 28, 2016

gartung changed the title ~~RecoTracker/MeasurementDet : gcc 6.0 misleading-indentation warning flags potential bug; with formatting and bug fix~~ RecoTracker/MeasurementDet : formatting fix for gcc 6.0 misleading-indentation warning Jul 1, 2016

gartung changed the title ~~RecoTracker/MeasurementDet : formatting fix for gcc 6.0 misleading-indentation warning~~ RecoTracker/MeasurementDet : bug fix for gcc 6.0 misleading-indentation warning Jul 1, 2016

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Jul 6, 2016

davidlange6 merged commit b64a650 into cms-sw:CMSSW_8_1_X Jul 9, 2016

gartung deleted the RecoTracker-MeasurementDet-fix-indent-not-guard-warn branch July 20, 2016 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RecoTracker/MeasurementDet : bug fix for gcc 6.0 misleading-indentation warning #15003

RecoTracker/MeasurementDet : bug fix for gcc 6.0 misleading-indentation warning #15003

gartung commented Jun 27, 2016 •

edited

cmsbuild commented Jun 27, 2016

slava77 commented Jun 27, 2016

cmsbuild commented Jun 27, 2016 •

edited

cmsbuild commented Jun 27, 2016

VinInn commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

gartung commented Jun 28, 2016

VinInn commented Jun 28, 2016

gartung commented Jun 28, 2016

gartung commented Jun 28, 2016

Dr15Jones Jun 28, 2016

slava77 Jun 28, 2016

Dr15Jones commented Jun 28, 2016

Dr15Jones commented Jun 28, 2016

gartung commented Jun 28, 2016 •

edited

Dr15Jones commented Jun 28, 2016

VinInn commented Jun 28, 2016

VinInn commented Jun 28, 2016

VinInn commented Jun 28, 2016

cmsbuild commented Jun 28, 2016 •

edited

cmsbuild commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

cvuosalo commented Jul 1, 2016

gartung commented Jul 1, 2016

cvuosalo commented Jul 6, 2016

cmsbuild commented Jul 6, 2016

RecoTracker/MeasurementDet : bug fix for gcc 6.0 misleading-indentation warning #15003

RecoTracker/MeasurementDet : bug fix for gcc 6.0 misleading-indentation warning #15003

Conversation

gartung commented Jun 27, 2016 • edited

cmsbuild commented Jun 27, 2016

slava77 commented Jun 27, 2016

cmsbuild commented Jun 27, 2016 • edited

cmsbuild commented Jun 27, 2016

VinInn commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

gartung commented Jun 28, 2016

VinInn commented Jun 28, 2016

gartung commented Jun 28, 2016

gartung commented Jun 28, 2016

Dr15Jones Jun 28, 2016

Choose a reason for hiding this comment

slava77 Jun 28, 2016

Choose a reason for hiding this comment

Dr15Jones commented Jun 28, 2016

Dr15Jones commented Jun 28, 2016

gartung commented Jun 28, 2016 • edited

Dr15Jones commented Jun 28, 2016

VinInn commented Jun 28, 2016

VinInn commented Jun 28, 2016

VinInn commented Jun 28, 2016

cmsbuild commented Jun 28, 2016 • edited

cmsbuild commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

cmsbuild commented Jun 28, 2016

cvuosalo commented Jul 1, 2016

gartung commented Jul 1, 2016

cvuosalo commented Jul 6, 2016

cmsbuild commented Jul 6, 2016

gartung commented Jun 27, 2016 •

edited

cmsbuild commented Jun 27, 2016 •

edited

gartung commented Jun 28, 2016 •

edited

cmsbuild commented Jun 28, 2016 •

edited