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.

Sign up for GitHub

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

Jump to bottom

Fix calling dtor on an abstract class with non-virtual dtor #15148

Merged
merged 1 commit into from Jul 12, 2016
Merged

Fix calling dtor on an abstract class with non-virtual dtor #15148

merged 1 commit into from Jul 12, 2016

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Jul 8, 2016

Clang pre-3.9 fails with an error:

In file included from src/TrackingTools/GeomPropagators/src/Aq:12:
TrackingTools/GeomPropagators/interface/OptimalHelixPlaneCrossing.h:42:34: error: destructor called on 'HelixPlaneCrossing' that is abstract but has non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
  ~OptimalHelixPlaneCrossing() { get()->~Base(); }
                                 ^
src/TrackingTools/GeomPropagators/interface/OptimalHelixPlaneCrossing.h:42:41: note: qualify call to silence this warning
  ~OptimalHelixPlaneCrossing() { get()->~Base(); }
                                        ^
                                        HelixPlaneCrossing::
1 error generated.

The changeset in Clang: http://reviews.llvm.org/D16206

Quote from C++ Standard used in this Clang changeset:

C++ [expr.delete]p3:
  In the first alternative (delete object), if the static type of the
  object to be deleted is different from its dynamic type, the static
  type shall be a base class of the dynamic type of the object to be
  deleted and the static type shall have a virtual destructor or the
  behavior is undefined.

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

@cmsbuild cmsbuild added this to the Next CMSSW_8_1_X milestone Jul 8, 2016
@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016

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

It involves the following packages:

TrackingTools/GeomPropagators

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @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

@davidlt
Copy link
Contributor Author

davidlt commented Jul 8, 2016

DEVEL IBs contain LLVM/Clang/LDD/RT/etc built from master branch to work on CMSSW + C++ Modules.

So, to avoid undefined behaviour one can also probably add virtual dtor to the class. Up to the experts to decide how to resolve this.

@VinInn
Copy link
Contributor

VinInn commented Jul 8, 2016

yes, HelixPlaneCrossing needs a virtual destructor... (It is pure virtual)
do you mind to add one in this very same PR?

@davidlt davidlt force-pushed the fix-compile-clang39 branch 2 times, most recently from 63b4f08 to 0fa9ffd Compare July 8, 2016 12:56
@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016

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

@davidlt
Copy link
Contributor Author

davidlt commented Jul 8, 2016

Updated with virtual dtor.

@VinInn
Copy link
Contributor

VinInn commented Jul 8, 2016

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016 •
edited

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016

Pull request #15148 was updated. @dmitrijus, @cvuosalo, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016

+1
Tested at: 0fa9ffd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15148/13945/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016

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

@smuzaffar
Copy link
Contributor

sorry, in order to get the IB pages recognize latest IBs I had to amend the e495908 commit to have message like "Merge CMSSW_8_0_X into CMSSW_8_1_X.". Due to that now to old commit is shows as part of this PR. either ignore it (it is already part of IB) or please rebase based on CMSSW_8_1_X head.

you can start from CMSSW_8_1_X and cherry pick the commits.

Fix calling dtor on an abstract class with non-virtual dtor
6319f5d 
Clang pre-3.9 fails with an error:

    In file included from src/TrackingTools/GeomPropagators/src/Aq:12:
    TrackingTools/GeomPropagators/interface/OptimalHelixPlaneCrossing.h:42:34: error: destructor called on 'HelixPlaneCrossing' that is abstract but has non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
      ~OptimalHelixPlaneCrossing() { get()->~Base(); }
                                     ^
    src/TrackingTools/GeomPropagators/interface/OptimalHelixPlaneCrossing.h:42:41: note: qualify call to silence this warning
      ~OptimalHelixPlaneCrossing() { get()->~Base(); }
                                            ^
                                            HelixPlaneCrossing::
    1 error generated.

The changeset in Clang: http://reviews.llvm.org/D16206

Quote from C++ Standard used in this Clang changeset:

    C++ [expr.delete]p3:
      In the first alternative (delete object), if the static type of the
      object to be deleted is different from its dynamic type, the static
      type shall be a base class of the dynamic type of the object to be
      deleted and the static type shall have a virtual destructor or the
      behavior is undefined.

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

cmsbuild commented Jul 8, 2016

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

@cvuosalo
Copy link
Contributor

cvuosalo commented Jul 8, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016 •
edited

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2016

+1
Tested at: 6319f5d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15148/13954/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2016

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

@cvuosalo
Copy link
Contributor

+1

For #15148 6319f5d

Adding a virtual destructor to HelixPlaneCrossing to silence a CLANG error. There should be no change in run-time behavior.

The code fix is satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-07-08-1700 show no significant differences, as expected.

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 2a27640 into cms-sw:CMSSW_8_1_X Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
comparison-available fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
Next CMSSW_8_1_X
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@davidlt @cmsbuild @VinInn @smuzaffar @cvuosalo @davidlange6