Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-13385

Remove unnecessary virtual destructors

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • afw
    • 1
    • Architecture
    • No

    Description

      In the course of the review of DM-9932, I noticed a number of classes with virtual destructors that probably don't need them (I think just due to the paranoia many new C++ developers have when they first learn about destructors; these are all very old classes). Fixing that is out of scope for DM-9932, but we should eventually look more carefully at each case and then remove them.

      The classes in question:

      • Exposure
      • ImagePca
      • ImageBase
      • Image
      • Mask
      • BackgroundControl

      Attachments

        Issue Links

          Activity

            tjenness Tim Jenness added a comment -

            Have these all been cleaned up? I know that wittgen has been doing lots of cleanups in afw.

            tjenness Tim Jenness added a comment - Have these all been cleaned up? I know that wittgen has been doing lots of cleanups in afw.

            I'll check.

            wittgen Matthias Wittgen added a comment - I'll check.
            • Exposure
            • ImagePca
            • ImageBase
            • ImageSlice
            • Image
            • Mask
            • BackgroundControl

            don't need virtual dtors, needed to remove some virtual operators/member functions.

            wittgen Matthias Wittgen added a comment - Exposure ImagePca ImageBase ImageSlice Image Mask BackgroundControl don't need virtual dtors, needed to remove some virtual operators/member functions.

            Can you explain what criteria you're using? ImageBase is a particularly odd choice since (as the name implies) it's intended for use as a base class.

            krzys Krzysztof Findeisen added a comment - Can you explain what criteria you're using? ImageBase is a particularly odd choice since (as the name implies) it's intended for use as a base class.
            wittgen Matthias Wittgen added a comment - - edited

            You are correct. This is not ready yet as this is more complex and needs evaluation of usages in other repos.

            wittgen Matthias Wittgen added a comment - - edited You are correct. This is not ready yet as this is more complex and needs evaluation of usages in other repos.

            ImageBase is non virtual other classes statically inherit from it

            • ImageBase -> Image -> ImageSlice (not inherited from, non-virtual)
            • ImageBase -> Mask (not inherited from, non-virtual
            • Exposure (not inherited from, non-virtual)
            • ImagePca (inherited from, has virtual functions, needs virtual dtor, wrongly stated in lists above )
            • BackgroundControl (not inherited from, non-virtual)}}

            Given the static polymorphism no virtual dtors are required. Only ImagePca needs a virtual dtor

             

            wittgen Matthias Wittgen added a comment - ImageBase is non virtual other classes statically inherit from it ImageBase -> Image -> ImageSlice (not inherited from, non-virtual) ImageBase -> Mask (not inherited from, non-virtual Exposure (not inherited from, non-virtual) ImagePca (inherited from, has virtual functions, needs virtual dtor, wrongly stated in lists above ) BackgroundControl (not inherited from, non-virtual)}} Given the static polymorphism no virtual dtors are required. Only  ImagePca needs a virtual dtor  
            wittgen Matthias Wittgen added a comment - Passed Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/38076/pipeline

            Overlooked that ImageSlice and Mask are used or might be used from their virtual base class Image, do
            a virtual dtor is needed.

            wittgen Matthias Wittgen added a comment - Overlooked that ImageSlice and Mask are used or might be used from their virtual base class Image , do a virtual dtor is needed.

            People

              wittgen Matthias Wittgen
              jbosch Jim Bosch
              Krzysztof Findeisen
              Jim Bosch, Krzysztof Findeisen, Matthias Wittgen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.