-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RecoLocalTracker/SiStripRecHitConverter: Fix bug found by clang warningand fix clang warnings: #22533
RecoLocalTracker/SiStripRecHitConverter: Fix bug found by clang warningand fix clang warnings: #22533
Conversation
…ng unincremented loop counter and fix clang warnings: src/RecoLocalTracker/SiStripRecHitConverter/src/StripCPE.cc /build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/llvm/5.0.0-ocgmcn/bin/clang++ -c -DGNU_GCC -D_GNU_SOURCE -DCMSSW_GIT_HASH='CMSSW_10_1_CLANG_X_2018-03-07-2300' -DPROJECT_NAME='CMSSW' -DPROJECT_VERSION='CMSSW_10_1_CLANG_X_2018-03-07-2300' -DTBB_USE_GLIBCXX_VERSION=60300 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -I/build/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/81541827a6a1dcc2a8e68c83e751c3d9/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_1_CLANG_X_2018-03-07-2300/src -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/lcg/root/6.10.09-omkpbe3/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/boost/1.63.0-ocgmcn/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/pcre/8.37-oenich/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/bz2lib/1.0.6/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/clhep/2.4.0.0-ocgmcn/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/gsl/2.2.1/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/libuuid/2.22.2/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/python/2.7.11-ocgmcn/include/python2.7 -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/tbb/2018_U1/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/cms/vdt/0.4.0/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/xerces-c/3.1.3/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/xz/5.2.2-oenich/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/zlib-x86_64/1.2.11/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/md5/1.0.0/include -I/build/cmsbld/jenkins/workspace/build-any-ib/w/slc6_amd64_gcc630/external/tinyxml/2.5.3-ocgmcn/include -O2 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++1z -ftree-vectorize -Wstrict-overflow -Werror=array-bounds -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -msse3 -felide-constructors -fmessage-length=0 -Wall -Wno-long-long -Wreturn-type -Wunused -Wparentheses -Wno-deprecated -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=strict-aliasing -Werror=narrowing -Werror=uninitialized -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-c99-extensions -Wno-c++11-narrowing -D__STRICT_ANSI__ -Wno-unused-private-field -Wno-unknown-pragmas -Wno-unused-command-line-argument -ftemplate-depth=512 -Wno-error=potentially-evaluated-expression -DBOOST_DISABLE_ASSERTS -Wno-error=unused-variable -Wno-error=unused-variable -Wno-error=unused-variable -fPIC -MMD -MF tmp/slc6_amd64_gcc630/src/RecoLocalTracker/SiStripRecHitConverter/src/RecoLocalTrackerSiStripRecHitConverter/StripCPE.d /build/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/81541827a6a1dcc2a8e68c83e751c3d9/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_1_CLANG_X_2018-03-07-2300/src/RecoLocalTracker/SiStripRecHitConverter/src/StripCPE.cc -o tmp/slc6_amd64_gcc630/src/RecoLocalTracker/SiStripRecHitConverter/src/RecoLocalTrackerSiStripRecHitConverter/StripCPE.o /build/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/81541827a6a1dcc2a8e68c83e751c3d9/opt/cmssw/slc6_amd64_gcc630/cms/cmssw/CMSSW_10_1_CLANG_X_2018-03-07-2300/src/RecoLocalTracker/SiStripRecHitConverter/src/SiStripTemplate.cc:555:16: warning: variable 'l' used in loop condition not modified in loop body [-Wfor-loop-analysis] for (l=0; l<TSXSIZE; ++k) {db >> theCurrentTemp.entx[k][i].xtemp[j][l]; qavg_avg += theCurrentTemp.entx[k][i].xtemp[j][l];} RecoLocalTracker/SiStripRecHitConverter/interface/StripCPEgeometric.h:13:5: warning: 'StripCPEgeometric::localParameters' hides overloaded virtual functions [-Woverloaded-virtual] localParameters( const SiStripCluster&, const GeomDetUnit&, const LocalTrajectoryParameters&) const override; ^ src/RecoLocalTracker/SiStripRecHitConverter/interface/StripCPEfromTemplate.h:17:5: warning: 'StripCPEfromTemplate::localParameters' hides overloaded virtual functions [-Woverloaded-virtual] localParameters( const SiStripCluster&, const GeomDetUnit&, const LocalTrajectoryParameters&) const override; ^
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22533/3850 |
please test |
The tests are being triggered in jenkins. |
A new Pull Request was created by @gartung (Patrick Gartung) for master. It involves the following packages: RecoLocalTracker/SiStripRecHitConverter @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@@ -552,7 +552,7 @@ bool SiStripTemplate::pushfile(const SiPixelTemplateDBObject& dbobject, std::vec | |||
qavg_avg = 0.f; | |||
for (j=0; j<9; ++j) { | |||
|
|||
for (l=0; l<TSXSIZE; ++k) {db >> theCurrentTemp.entx[k][i].xtemp[j][l]; qavg_avg += theCurrentTemp.entx[k][i].xtemp[j][l];} | |||
for (l=0; l<TSXSIZE; ++l) {db >> theCurrentTemp.entx[k][i].xtemp[j][l]; qavg_avg += theCurrentTemp.entx[k][i].xtemp[j][l];} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix, while due, seems to possible affect the normal workflow of the code: when k was (erroneously) incremented, the wrong k entered the following checks inside the outer loop
@OlivierBondu : whenever there are no issues with what stored in the the db probably the previous bug didn't show up, Do you expect situations in which there is any effect of this fix? Where should we look at to possibly see one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is NOT such a thing as a StripTemplate in the database....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that this part of the code was never hit, and therefore while the fix is due we cannot see any effect out of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naively thought the effect of the bug should be an infinite loop (likely leading to a segfault at some point).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed... That's correct @makortel: this demonstrates that this code was never run!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look, and it seems there are two reasons why this did not give problems:
- the template CPE is not used for the strips (StripCPEfromTrackAngle is the default), and the template cfi is only imported in some outdated test configs
- there is no code in CMSSW that calls this method (the template CPE uses the one defined above,
bool SiStripTemplate::pushfile(int filenum, std::vector< SiStripTemplateStore > & theStripTemp_)
) - I verified by removing it and recompiling (aftergit cms-checkdeps -a
), which showed no problems.
According to git this has not been touched since the migration from CVS. I think it has been copied/adapted from the corresponding code for the pixel detector here at some point.
Question (also to @mmusich @echabert @boudoul ): since this is not used (and apparently never has been), and it can be revived from git history or the pixel version in case it is needed some day, should we just remove this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, @pieterdavid, @perrotta,
as you correctly point out Strip template reconstruction was never developed (same people working on the pixel one were involved here) up to a point where it could be used in production and certainly we don't even have CondFormat
to be able to store in DB. While we need to support the fromTrackAngle
and, up to a certain degree the geometric
CPE algorithm, I personally see no reason to keep in repository the template code at all if all it does is to create maintenance overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second @mmusich 's comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pieterdavid @mmusich @echabert @boudoul: will any of you take care of preparing a PR which removes from the release whatever is not used (in this package)? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
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) |
+1 |
Loop counter not incremented in body.
src/RecoLocalTracker/SiStripRecHitConverter/src/SiStripTemplate.cc:555:16: warning: variable 'l' used in loop condition not modified in loop body [-Wfor-loop-analysis]
for (l=0; l<TSXSIZE; ++k) {db >> theCurrentTemp.entx[k][i].xtemp[j][l]; qavg_avg += theCurrentTemp.entx[k][i].xtemp[j][l];}
Warnings:
RecoLocalTracker/SiStripRecHitConverter/interface/StripCPEgeometric.h:13:5: warning: 'StripCPEgeometric::localParameters' hides overloaded virtual functions [-Woverloaded-virtual]
localParameters( const SiStripCluster&, const GeomDetUnit&, const LocalTrajectoryParameters&) const override;
^
src/RecoLocalTracker/SiStripRecHitConverter/interface/StripCPEfromTemplate.h:17:5: warning: 'StripCPEfromTemplate::localParameters' hides overloaded virtual functions [-Woverloaded-virtual]
localParameters( const SiStripCluster&, const GeomDetUnit&, const LocalTrajectoryParameters&) const override;
^