DataFormats/TrackerRecHit2D: Fix warning warning: 'class TkCloner' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor] #18927

gartung · 2017-05-25T01:32:37Z

This gcc 7.0.0 warning appears throughout the build log.

DataFormats/TrackerRecHit2D/interface/TkCloner.h:15:7: warning: 'class TkCloner' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]

Fixed by adding virtual destructor for TkCloner.

cmsbuild · 2017-05-25T01:32:56Z

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

It involves the following packages:

DataFormats/TrackerRecHit2D

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @gpetruc, @ebrondol, @VinInn this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

gartung · 2017-05-25T01:34:14Z

@Dr15Jones

Dr15Jones · 2017-05-25T02:34:56Z

Please test

cmsbuild · 2017-05-25T02:35:11Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20086/console Started: 2017/05/25 06:50

VinInn · 2017-05-25T07:44:31Z

Could you stop adding virtual destructor to classes whose instances are never deleted through a base class pointer?
(and please do not add random ";" and if the destructor is default use "=default;"

C++ is a language for those who know what they are doing, we should teach rules, not impose random syntax rules.

cmsbuild · 2017-05-25T07:45:48Z

-1

Tested at: e8f5250

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18927/20086/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests RelVals AddOn

Unit Tests:

I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS

RelVals:

When I ran the RelVals I found an error in the following worklfows:
4.53 step2

runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT/step2_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT.log

136.731 step2

runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step2_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log

AddOn:

I found errors in the following addon tests:

cmsDriver.py RelVal -s L1REPACK:Full --data --scenario=pp -n 10 --conditions auto:run2_hlt_PRef --relval 9000,50 --datatier "RAW" --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_2017 --magField 38T_PostLS1 --eventcontent RAW --fileout file:RelVal_Raw_PRef_DATA.root --filein /store/data/Run2016B/JetHT/RAW/v1/000/272/762/00000/C666CDE2-E013-E611-B15A-02163E011DBE.root : FAILED - time: date Thu May 25 09:28:08 2017-date Thu May 25 09:18:46 2017 s - exit: 23552
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02473/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_2_X_2017-05-24-2300/src/HLTrigger/Configuration/test/OnLine_HLT_PRef.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Thu May 25 09:28:08 2017-date Thu May 25 09:18:46 2017 s - exit: 21504
cmsDriver.py RelVal -s HLT:PRef,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run2_data_PRef --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2017 --magField 38T_PostLS1 --processName=HLTRECO --filein file:RelVal_Raw_PRef_DATA.root --fileout file:RelVal_Raw_PRef_DATA_HLT_RECO.root : FAILED - time: date Thu May 25 09:28:08 2017-date Thu May 25 09:18:46 2017 s - exit: 21504
cmsDriver.py RelVal -s L1REPACK:Full2015Data --data --scenario=HeavyIons -n 10 --conditions auto:run2_hlt_HIon --relval 9000,50 --datatier "RAW" --eventcontent RAW --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_2016,Run2_HI --magField 38T_PostLS1 --fileout file:RelVal_Raw_HIon_DATA.root --filein /store/hidata/HIRun2015/HIHardProbes/RAW-RECO/HighPtJet-PromptReco-v1/000/263/689/00000/1802CD9A-DDB8-E511-9CF9-02163E0138CA.root : FAILED - time: date Thu May 25 09:28:08 2017-date Thu May 25 09:18:50 2017 s - exit: 23552
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02473/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_2_X_2017-05-24-2300/src/HLTrigger/Configuration/test/OnLine_HLT_HIon.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Thu May 25 09:28:08 2017-date Thu May 25 09:18:50 2017 s - exit: 21504
cmsDriver.py RelVal -s HLT:HIon,RAW2DIGI,L1Reco,RECO --data --scenario=HeavyIons -n 10 --conditions auto:run2_data_HIon --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2016,Run2_HI --magField 38T_PostLS1 --processName=HLTRECO --filein file:RelVal_Raw_HIon_DATA.root --fileout file:RelVal_Raw_HIon_DATA_HLT_RECO.root : FAILED - time: date Thu May 25 09:28:08 2017-date Thu May 25 09:18:50 2017 s - exit: 21504
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02473/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_2_X_2017-05-24-2300/src/Utilities/ReleaseScripts/scripts/read312RV_cfg.py : FAILED - time: date Thu May 25 09:31:19 2017-date Thu May 25 09:28:09 2017 s - exit: 23552
cmsDriver.py RelVal -s L1REPACK:GT1 --data --scenario=pp -n 10 --conditions auto:run1_hlt_Fake --relval 9000,50 --datatier "RAW" --eventcontent RAW --customise=HLTrigger/Configuration/CustomConfigs.L1T --fileout file:RelVal_Raw_Fake_DATA.root --filein /store/data/Run2012A/MuEG/RAW/v1/000/191/718/14932935-E289-E111-830C-5404A6388697.root : FAILED - time: date Thu May 25 09:32:13 2017-date Thu May 25 09:28:10 2017 s - exit: 23552
cmsRun /cvmfs/cms-ib.cern.ch/nweek-02473/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_2_X_2017-05-24-2300/src/HLTrigger/Configuration/test/OnLine_HLT_Fake.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Thu May 25 09:32:13 2017-date Thu May 25 09:28:10 2017 s - exit: 21504
cmsDriver.py RelVal -s HLT:Fake,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run1_data_Fake --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --processName=HLTRECO --filein file:RelVal_Raw_Fake_DATA.root --fileout file:RelVal_Raw_Fake_DATA_HLT_RECO.root : FAILED - time: date Thu May 25 09:32:13 2017-date Thu May 25 09:28:10 2017 s - exit: 21504

cmsbuild · 2017-05-25T07:45:50Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

Dr15Jones · 2017-05-25T12:13:13Z

@VinInn I asked for Patrick to do these changes. The gcc7 build has thousands of warnings stemming from base classes not declaring virtual destructors. In addition, CMS coding standards do say that one should declare a virtual destructor for all classes used as base classes. I understand that the way one uses a class now may not require a virtual destructor, but if someone changes the use (say holds a std::vector<std::shared_ptr<Foo>>) then the code will not work properly. This change future proofs the code.

VinInn · 2017-05-25T12:19:31Z

There are specific cases where this is not required and actually causes measurable performance degradation. This is NOT one of those, I agree (still please fix the grammar).
In particular I plan to use stack-allocated instances of classes with "nop" destructors and I do not want to see neither the destruction in action nor to pay the price of the virtual table (in case of almost-PODs)

…nctions and accessible non-virtual destructor [-Wnon-virtual-dtor]

cmsbuild · 2017-05-25T12:23:55Z

Pull request #18927 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

gartung · 2017-05-25T12:24:38Z

It this OK?

VinInn · 2017-05-25T12:26:19Z

DataFormats/TrackerRecHit2D/interface/TkCloner.h

@@ -18,6 +18,7 @@ class TkCloner {
    return hit.clone(*this, tsos);
  }

+ virtual ~TkCloner() {}


better... thanks.

gartung · 2017-05-25T12:43:54Z

I initiall had
virtual ~TkCloner() = default;
a Chris suggested but I ran into errors with classes that declared a read destructor. To be safe I changed this one to {} and forgot to remove the ;
I switch between python, perl, c++ and shell so much that I get the syntax rules mixed up.

perrotta · 2017-05-25T14:22:05Z

please test

cmsbuild · 2017-05-25T14:22:21Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20091/console Started: 2017/05/25 16:23

cmsbuild · 2017-05-25T16:23:17Z

+1
Tested at: f74b293
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18927/20091/summary.html

cmsbuild · 2017-05-25T16:23:20Z

Comparison job queued.

cmsbuild · 2017-05-25T19:12:00Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1673 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1833024
DQMHistoTests: Total failures: 57125
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1775719
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

perrotta · 2017-05-26T13:30:33Z

+1

technical fix,
jenkins tests pass
if c++ gurus endorse it, it is also fine for reco

cmsbuild · 2017-05-26T13:30:51Z

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

davidlange6 · 2017-05-27T07:43:46Z

+1

cmsbuild added this to the CMSSW_9_2_X milestone May 25, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 25, 2017

cmsbuild added tests-started and removed tests-pending labels May 25, 2017

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels May 25, 2017

DataFormats/TrackerRecHit2D: fix gcc700 warning: class has virtual fu…

f74b293

…nctions and accessible non-virtual destructor [-Wnon-virtual-dtor]

cmsbuild added comparison-pending tests-pending and removed comparison-notrun tests-rejected labels May 25, 2017

VinInn reviewed May 25, 2017

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels May 25, 2017

cmsbuild added tests-approved and removed tests-started labels May 25, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 25, 2017

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels May 26, 2017

cmsbuild added orp-approved and removed orp-pending labels May 27, 2017

cmsbuild merged commit d9d4e78 into cms-sw:master May 27, 2017

gartung deleted the DataFormats-TrackerRecHit2D-gcc700-dtor-warning-fix branch May 30, 2017 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataFormats/TrackerRecHit2D: Fix warning warning: 'class TkCloner' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor] #18927

DataFormats/TrackerRecHit2D: Fix warning warning: 'class TkCloner' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor] #18927

gartung commented May 25, 2017 •

edited

cmsbuild commented May 25, 2017

gartung commented May 25, 2017

Dr15Jones commented May 25, 2017

cmsbuild commented May 25, 2017 •

edited

VinInn commented May 25, 2017

cmsbuild commented May 25, 2017

cmsbuild commented May 25, 2017

Dr15Jones commented May 25, 2017

VinInn commented May 25, 2017

cmsbuild commented May 25, 2017

gartung commented May 25, 2017

VinInn May 25, 2017

gartung commented May 25, 2017

perrotta commented May 25, 2017

cmsbuild commented May 25, 2017 •

edited

cmsbuild commented May 25, 2017

cmsbuild commented May 25, 2017

cmsbuild commented May 25, 2017

perrotta commented May 26, 2017

cmsbuild commented May 26, 2017

davidlange6 commented May 27, 2017

DataFormats/TrackerRecHit2D: Fix warning warning: 'class TkCloner' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor] #18927

DataFormats/TrackerRecHit2D: Fix warning warning: 'class TkCloner' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor] #18927

Conversation

gartung commented May 25, 2017 • edited

cmsbuild commented May 25, 2017

gartung commented May 25, 2017

Dr15Jones commented May 25, 2017

cmsbuild commented May 25, 2017 • edited

VinInn commented May 25, 2017

cmsbuild commented May 25, 2017

cmsbuild commented May 25, 2017

Dr15Jones commented May 25, 2017

VinInn commented May 25, 2017

cmsbuild commented May 25, 2017

gartung commented May 25, 2017

VinInn May 25, 2017

Choose a reason for hiding this comment

gartung commented May 25, 2017

perrotta commented May 25, 2017

cmsbuild commented May 25, 2017 • edited

cmsbuild commented May 25, 2017

cmsbuild commented May 25, 2017

cmsbuild commented May 25, 2017

perrotta commented May 26, 2017

cmsbuild commented May 26, 2017

davidlange6 commented May 27, 2017

gartung commented May 25, 2017 •

edited

cmsbuild commented May 25, 2017 •

edited

cmsbuild commented May 25, 2017 •

edited