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

Really make reco::TrackBase destructor virtual #187

Merged
merged 2 commits into from Jul 30, 2013

Conversation

wmtan
Copy link
Contributor

@wmtan wmtan commented Jul 25, 2013

Back in January 2008, an attempt was made to add a virtual destructor to reco::TrackBase and its derived classes, such as reco::Track. Unfortunately, the virtual keyword was not added in TrackBase, just a comment stating (incorrectly) that the destructor was virtual. This is a bug, as the class needs a virtual destructor.
This request merely adds the virtual keyword to the destructor to fix the bug. It was tested with checkdeps -a, and the unit tests pass.

This is a trivial C++ bug fix that has nothing really to do with RECO, so if it is not signed in a timely manner, I suggest that the signature be bypassed.

The bug causes a unit test failure in CommonTools/Utils when the Reflex functionality for invoking functions is replaced by ROOT. So, this fix is needed.

@nclopezo
Copy link
Contributor

Hi,

I took CMSSW_7_0_X_2013-07-25-1400, pulled these changes, and when I was building I got an error for the package Fireworks/Core:

>> Compiling  /build/dmendezl/CMSSW_7_0_X_2013-07-25-1400/src/Fireworks/Calo/src/thetaBins.cc 
>> Building shared library tmp/slc5_amd64_gcc472/src/Fireworks/Core/src/FireworksCore/libFireworksCore.so
>> Compiling  /build/dmendezl/CMSSW_7_0_X_2013-07-25-1400/src/Fireworks/Muons/src/FWMuonBuilder.cc 
tmp/slc5_amd64_gcc472/src/Fireworks/Core/src/FireworksCore/FWInvMassDialog.o:FWInvMassDialog.cc:function FWInvMassDialog::Calculate(): error: undefined reference to 'typeinfo for reco::TrackBase'
collect2: error: ld returned 1 exit status

The error comes from the file FWInvMassDialog.cc, you can look at line 133 or 161:

https://cmssdt.cern.ch/SDT/lxr/source/Fireworks/Core/src/FWInvMassDialog.cc?v=CMSSW_7_0_X_2013-07-29-0200#133

@Dr15Jones
Copy link
Contributor

I've seen similar problems where SCRAM has failed to recompile some piece of dependent code. When I've done 'scram b clean' then 'scram b' the problem goes away. How clean of a build area did you start from?

@wmtan
Copy link
Contributor Author

wmtan commented Jul 29, 2013

The build error is real. I did a full build before doing the pull request, but apparently I did not look carefully enough at the log file. The error is there. I will fix it.

@wmtan
Copy link
Contributor Author

wmtan commented Jul 29, 2013

The pull request now links properly. There was a missing link dependency in the build file in Fireworks/Core. I added it, and the build now works. I updated the pull request.

@nclopezo
Copy link
Contributor

Hi,

I pulled these changes on top of CMSSW_7_0_X_2013-07-29-1400, I built it and then ran the unit tests and the RelVals. All the tests passed.

@wmtan
Copy link
Contributor Author

wmtan commented Jul 30, 2013

I would request that unless this request is signed promptly, that the signature be bypassed. It is a purely technical C++ fix that has nothing really to do with reconstruction.

nclopezo added a commit that referenced this pull request Jul 30, 2013
Really make reco::TrackBase destructor virtual
@nclopezo nclopezo merged commit 74f7c9a into cms-sw:CMSSW_7_0_X Jul 30, 2013
tahuang1991 pushed a commit to tahuang1991/cmssw that referenced this pull request Sep 15, 2014
csc trackfinder: allow data and MC syncronization
nclopezo pushed a commit to nclopezo/cmssw that referenced this pull request Nov 21, 2014
Update watchers.yaml for echapon
parbol pushed a commit to parbol/cmssw that referenced this pull request Mar 5, 2015
TimeReport functionality, and heppy bugfixes.
arizzi added a commit to arizzi/cmssw that referenced this pull request Sep 30, 2015
mariadalfonso added a commit to mariadalfonso/cmssw that referenced this pull request Dec 18, 2017
Training with some kinematic variables.
cmsbuild pushed a commit that referenced this pull request Mar 10, 2018
fwyzard added a commit to rovere/cmssw that referenced this pull request Oct 7, 2018
slava77 pushed a commit to slava77/cmssw that referenced this pull request Oct 9, 2021
emily-tsai11 pushed a commit to emily-tsai11/cmssw that referenced this pull request Nov 15, 2022
rgoldouz pushed a commit to rgoldouz/cmssw that referenced this pull request Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants