Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:
-
Story Points:1
-
Team:Architecture
-
Urgent?: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
- mentioned in
-
Page Loading...
Activity
I'll check.
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.
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.
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. You are correct. This is not ready yet as this is more complex and needs evaluation of usages in other repos.
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
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
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.
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. 
Have these all been cleaned up? I know that Matthias Wittgen has been doing lots of cleanups in afw.