Skip to content

[TrigConfiguration] Clean up destructor definitions

Rafal Bielski requested to merge rbielski/athena:trigconf-destructors into master

There are many changes in this MR but all are trivial and of the same kind. Adjust all classes in the TrigConfiguration domain to follow the destructor definition rules outlined below. I focused on this domain since it is core framework code (so should serve as an example of good code) and involves a lot of inheritance where this is relevant.

The rules followed are:

  1. If the class is not involved in inheritance and is unlikely to be (e.g. defined as final or just a simple one use-case class without any virtual methods) and does not require a custom destructor, do not explicitly declare the destructor. This follows the "rule of zero". [1][2]
  2. If the class is a base class for other classes (or may be and has virtual methods) and does not require a custom destructor, declare it as virtual ~MyClass() = default;. It is required to be defined to avoid undefined behaviour [3][4] and it is recommended to use = default for trivial destructors. [5][6]
  3. If the class is derived, declare the destructor as virtual ~MyClass() override;. The override here is a clear additional indication of the inheritance and enforces that the base class has a virtual destructor defined, which prevents a class of bugs [7-10]. The virtual is redundant in this case [10][11] but arguably using both virtual and override may be useful and is actually recommended by ATLAS [12] as well as widespread and standard in the athena repository.
  4. In addition to 3. if no custom implementation is required, declare the destructor as virtual ~MyClass() override = default;. This is the most common change in this MR.

On top of everything, adding the override to destructors uncovers other missing overrides in many classes since it triggers the [-Winconsistent-missing-override] compilation warning for classes where the override was incorrectly but consistently missing in multiple methods. Another good reason why this MR improves the code quality. The uncovered missing overrides were fixed in !51739 (merged).


[1] https://en.cppreference.com/w/cpp/language/rule_of_three
[2] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-zero
[3] https://en.cppreference.com/w/cpp/language/destructor
[4] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual
[5] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#example-good-6
[6] https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html
[7] https://stackoverflow.com/questions/17923370/override-identifier-after-destructor-in-c11
[8] https://github.com/isocpp/CppCoreGuidelines/pull/1448
[9] https://github.com/isocpp/CppCoreGuidelines/issues/721
[10] https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
[11] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-override
[12] [redeclare-virtual] in https://atlas-computing.web.cern.ch/atlas-computing/projects/qa/draft_guidelines.html#orgbb16391

Edited by Rafal Bielski

Merge request reports