refactor: Clang tidy fixes (#1308)

This PR adds fixes for

- readability-container-size-empty
- modernize-use-override
- modernize-use-equals-default
- readability-implicit-bool-cast
- readability-implicit-bool-conversion

because they're fairly easy to do, and can be problematic.

Also improves the reporting. #1306 should be merged first.

.clang-tidy

@@ -1,4 +1,4 @@
 ---
-Checks: '-*,readability-*,-readability-redundant-member-init,misc-*,-misc-unused-parameters,bugprone-*,performance-*,modernize-*,-modernize-use-auto,-modernize-use-trailing-return-type,clang-analyzer-deadcode.*,clang-analyzer-*,-clang-analyzer-osx.*,-clang-analyzer-unix.*,cppcoreguidelines-*,-cppcoreguidelines-pro-type-vararg,-cppcoreguidelines-owning-memory,-cppcoreguidelines-pro-bounds-constant-array'
+Checks: '-*,readability-container-size-empty,readability-implicit-bool-cast,readability-implicit-bool-conversion,modernize-use-equals-default,modernize-use-override'
 HeaderFilterRegex: '.*(?<!nlohmann\/json)\.(hpp|cpp|ipp)$'
 AnalyzeTemporaryDtors: true

.gitlab-ci.yml

@@ -32,7 +32,7 @@ clang_tidy:
     - mkdir clang-tidy
     - CI/clang_tidy/run_clang_tidy.sh build > clang-tidy/clang-tidy.log
     - pip install -r CI/clang_tidy/requirements.txt
-    - CI/clang_tidy/parse_clang_tidy.py clang-tidy/clang-tidy.log clang-tidy/clang-tidy.json
+    - CI/clang_tidy/parse_clang_tidy.py clang-tidy/clang-tidy.log clang-tidy/clang-tidy.json --exclude "*thirdparty*"
     - CI/clang_tidy/check_clang_tidy.py --report clang-tidy/clang-tidy.json --config CI/clang_tidy/limits.yml
     - codereport clang-tidy/clang-tidy.json clang-tidy/html
 

CI/clang_tidy/check_clang_tidy.py

@@ -46,8 +46,6 @@ def main():
 
         output = []
         for item in items:
-            if item.code in config["ignore"]:
-                continue
 
             emoji = Emoji(
                 {"warning": "yellow_circle", "error": "red_circle"}[item.severity]
@@ -67,23 +65,31 @@ def main():
             s.append(item.code, style="bold")
             s.append(f"]")
 
-            output.append(s)
-
             def subpath(m):
                 return f"[bold]{m.group(1)}[/bold]:"
 
             message = re.sub(r"([\w/.\-+]+:\d+:\d+):", subpath, item.message)
-            output.append(Panel(message))
-            output.append(Rule())
 
-            for pattern in counts.keys():
+            accounted_for = False
+            for pattern in config["limits"].keys():
 
                 if not fnmatch.fnmatch(item.code, pattern):
                     continue
                 counts[pattern] += 1
+                accounted_for = True
+
+            if accounted_for:
+                output.append(s)
+                output.append(Panel(message))
+                output.append(Rule())
+                pass
+            else:
+                counts.setdefault(item.code, 0)
+                counts[item.code] += 1
 
-        output = output[:-1]
-        console.print(Panel(Group(*output), title=str(file)))
+        #  output = output[:-1]
+        if len(output) > 0:
+            console.print(Panel(Group(*output), title=str(file)))
 
     table = Table()
     table.add_column("", width=2)
@@ -92,10 +98,13 @@ def subpath(m):
     table.add_column("limit", justify="right")
     exit = 0
     for pattern, count in counts.items():
-        limit = config["limits"][pattern]
+        limit = config["limits"].get(pattern, float("inf"))
         emoji = Emoji("green_circle")
         style = "green"
-        if count > limit:
+        if limit == float("inf"):
+            emoji = Emoji("white_circle")
+            style = "white"
+        elif count > limit:
             exit = 1
             emoji = Emoji("red_circle")
             style = "red bold"

CI/clang_tidy/limits.yml

@@ -1,16 +1,14 @@
-limits: {}
+limits:
   # "readability-inconsistent-declaration-parameter-name": 0
   # "readability-named-parameter": 0
-  # "readability-container-size-empty": 0
+  "readability-container-size-empty": 0
   # "modernize-use-using": 0
   #"readability-braces-around-statements": 0
-  # "modernize-use-override": 0
-  # "modernize-use-equals-default" : 0
-  # "readability-implicit-bool-cast": 0
+  "modernize-use-override": 0
+  "modernize-use-equals-default" : 0
+  "readability-implicit-bool-cast": 0
+  "readability-implicit-bool-conversion": 0
   # "modernize-use-default-member-init": 0
   # "performance-unnecessary-value-param": 0
   # "modernize-use-equals-default": 0
   # "modernize-use-nullptr": 0
-
-ignore:
-  - cppcoreguidelines-pro-type-vararg

Core/src/EventData/PrintParameters.cpp

@@ -165,7 +165,7 @@ void Acts::detail::printBoundParameters(std::ostream& os,
                                         const Acts::Surface& surface,
                                         const Acts::BoundVector& params,
                                         const Acts::BoundSymMatrix* cov) {
-  if (cov) {
+  if (cov != nullptr) {
     printParametersCovariance(os, makeBoundNames(), kMonotonic, params, *cov);
   } else {
     printParameters(os, makeBoundNames(), kMonotonic, params);
@@ -177,7 +177,7 @@ void Acts::detail::printBoundParameters(std::ostream& os,
 void Acts::detail::printFreeParameters(std::ostream& os,
                                        const Acts::FreeVector& params,
                                        const Acts::FreeMatrix* cov) {
-  if (cov) {
+  if (cov != nullptr) {
     printParametersCovariance(os, makeFreeNames(), kMonotonic, params, *cov);
   } else {
     printParameters(os, makeFreeNames(), kMonotonic, params);

Core/src/Geometry/GeometryIdentifier.cpp

@@ -12,7 +12,7 @@
 
 std::ostream& Acts::operator<<(std::ostream& os, Acts::GeometryIdentifier id) {
   // zero represents an invalid/undefined identifier
-  if (not id.value()) {
+  if (id.value() == 0u) {
     return (os << "undefined");
   }
 
@@ -25,7 +25,7 @@ std::ostream& Acts::operator<<(std::ostream& os, Acts::GeometryIdentifier id) {
 
   bool writeSeparator = false;
   for (auto i = 0u; i < 5u; ++i) {
-    if (levels[i]) {
+    if (levels[i] != 0u) {
       if (writeSeparator) {
         os << '|';
       }

Core/src/Geometry/Layer.cpp

@@ -117,7 +117,7 @@ Acts::Layer::compatibleSurfaces(
   // check if you have to stop at the endSurface
   double pathLimit = options.pathLimit;
   double overstepLimit = options.overstepLimit;
-  if (options.endObject) {
+  if (options.endObject != nullptr) {
     // intersect the end surface
     // - it is the final one don't use the bounday check at all
     SurfaceIntersection endInter = options.endObject->intersect(
@@ -149,7 +149,7 @@ Acts::Layer::compatibleSurfaces(
       return true;
     }
     // next option: it's a material surface and you want to have it
-    if (options.resolveMaterial && sf.surfaceMaterial()) {
+    if (options.resolveMaterial && sf.surfaceMaterial() != nullptr) {
       return true;
     }
     // last option: resovle all
@@ -260,7 +260,7 @@ Acts::SurfaceIntersection Acts::Layer::surfaceOnApproach(
   bool resolvePS = options.resolveSensitive || options.resolvePassive;
   bool resolveMS = options.resolveMaterial &&
                    (m_ssSensitiveSurfaces > 1 || m_ssApproachSurfaces > 1 ||
-                    surfaceRepresentation().surfaceMaterial());
+                    (surfaceRepresentation().surfaceMaterial() != nullptr));
 
   // The signed direction: solution (except overstepping) is positive
   auto sDirection = options.navDir * direction;

Core/src/Geometry/TrackingGeometry.cpp

@@ -23,7 +23,7 @@ Acts::TrackingGeometry::TrackingGeometry(
   m_volumesById.rehash(0);
   // fill surface lookup container
   m_world->visitSurfaces([this](const Acts::Surface* srf) {
-    if (srf) {
+    if (srf != nullptr) {
       m_surfacesById[srf->geometryId()] = srf;
     }
   });

Core/src/Geometry/TrackingVolume.cpp

@@ -372,7 +372,8 @@ void Acts::TrackingVolume::closeGeometry(
   if (materialDecorator != nullptr) {
     materialDecorator->decorate(*thisVolume);
   }
-  if (thisVolume->volumeMaterial() == nullptr && thisVolume->motherVolume() &&
+  if (thisVolume->volumeMaterial() == nullptr &&
+      thisVolume->motherVolume() != nullptr &&
       thisVolume->motherVolume()->volumeMaterial() != nullptr) {
     auto protoMaterial = dynamic_cast<const Acts::ProtoVolumeMaterial*>(
         thisVolume->motherVolume()->volumeMaterial());

Core/src/Visualization/GeometryView3D.cpp

@@ -40,8 +40,8 @@ std::string joinPaths(const std::string& a, const std::string& b) {
 
 std::string getWorkingDirectory() {
   char buffer[PATH_MAX];
-  return (getcwd(buffer, sizeof(buffer)) ? std::string(buffer)
-                                         : std::string(""));
+  return (getcwd(buffer, sizeof(buffer)) != nullptr ? std::string(buffer)
+                                                    : std::string(""));
 }
 
 }  // namespace

Examples/Algorithms/Alignment/src/AlignmentAlgorithmFunction.cpp

@@ -41,7 +41,7 @@ struct AlignmentFunctionImpl
       const ActsExamples::TrackParametersContainer& initialParameters,
       const ActsAlignment::AlignmentOptions<
           ActsExamples::AlignmentAlgorithm::TrackFitterOptions>& options)
-      const {
+      const override {
     return align.align(sourceLinks, initialParameters, options);
   };
 };

Examples/Algorithms/Digitization/src/DigitizationAlgorithm.cpp

@@ -143,7 +143,7 @@ ActsExamples::ProcessCode ActsExamples::DigitizationAlgorithm::execute(
     const Acts::Surface* surfacePtr =
         m_cfg.trackingGeometry->findSurface(moduleGeoId);
 
-    if (not surfacePtr) {
+    if (surfacePtr == nullptr) {
       // this is either an invalid geometry id or a misconfigured smearer
       // setup; both cases can not be handled and should be fatal.
       ACTS_ERROR("Could not find surface " << moduleGeoId

Examples/Algorithms/Digitization/src/PlanarSteppingAlgorithm.cpp

@@ -74,18 +74,18 @@ ActsExamples::PlanarSteppingAlgorithm::PlanarSteppingAlgorithm(
     Digitizable dg;
     // require a valid surface
     dg.surface = surface;
-    if (not dg.surface) {
+    if (dg.surface == nullptr) {
       return;
     }
     // require an associated detector element
     dg.detectorElement = dynamic_cast<const Acts::IdentifiedDetectorElement*>(
         dg.surface->associatedDetectorElement());
-    if (not dg.detectorElement) {
+    if (dg.detectorElement == nullptr) {
       return;
     }
     // require an associated digitization module
     dg.digitizer = dg.detectorElement->digitizationModule().get();
-    if (not dg.digitizer) {
+    if (dg.digitizer == nullptr) {
       return;
     }
     // record all valid surfaces
@@ -148,7 +148,7 @@ ActsExamples::ProcessCode ActsExamples::PlanarSteppingAlgorithm::execute(
           m_cfg.planarModuleStepper->cellSteps(ctx.geoContext, *dg.digitizer,
                                                localIntersect, localDirection);
       // everything under threshold or edge effects
-      if (!dSteps.size()) {
+      if (dSteps.empty()) {
         ACTS_VERBOSE("No steps returned from stepper.");
         continue;
       }

Examples/Algorithms/Fatras/src/FatrasSimulation.cpp

@@ -40,8 +40,8 @@ struct HitSurfaceSelector {
   /// Check if the surface should be used.
   bool operator()(const Acts::Surface &surface) const {
     // sensitive/material are not mutually exclusive
-    bool isSensitive = surface.associatedDetectorElement();
-    bool isMaterial = surface.surfaceMaterial();
+    bool isSensitive = surface.associatedDetectorElement() != nullptr;
+    bool isMaterial = surface.surfaceMaterial() != nullptr;
     // passive should be an orthogonal category
     bool isPassive = not(isSensitive or isMaterial);
     return (isSensitive and sensitive) or (isMaterial and material) or
@@ -141,7 +141,7 @@ struct FatrasSimulationT final : ActsExamples::detail::FatrasSimulation {
     simulation.charged.selectHitSurface.material = cfg.generateHitsOnMaterial;
     simulation.charged.selectHitSurface.passive = cfg.generateHitsOnPassive;
   }
-  ~FatrasSimulationT() final override = default;
+  ~FatrasSimulationT() final = default;
 
   Acts::Result<std::vector<ActsFatras::FailedParticle>> simulate(
       const Acts::GeometryContext &geoCtx,
@@ -151,8 +151,7 @@ struct FatrasSimulationT final : ActsExamples::detail::FatrasSimulation {
           &simulatedParticlesInitial,
       ActsExamples::SimParticleContainer::sequence_type
           &simulatedParticlesFinal,
-      ActsExamples::SimHitContainer::sequence_type &simHits)
-      const final override {
+      ActsExamples::SimHitContainer::sequence_type &simHits) const final {
     return simulation.simulate(geoCtx, magCtx, rng, inputParticles,
                                simulatedParticlesInitial,
                                simulatedParticlesFinal, simHits);
@@ -189,7 +188,7 @@ ActsExamples::FatrasSimulation::FatrasSimulation(Config cfg,
 }
 
 // explicit destructor needed for the PIMPL implementation to work
-ActsExamples::FatrasSimulation::~FatrasSimulation() {}
+ActsExamples::FatrasSimulation::~FatrasSimulation() = default;
 
 ActsExamples::ProcessCode ActsExamples::FatrasSimulation::execute(
     const AlgorithmContext &ctx) const {

Examples/Algorithms/Geant4/src/Geant4Simulation.cpp

@@ -119,7 +119,7 @@ ActsExamples::Geant4Simulation::Geant4Simulation(
   }
 }
 
-ActsExamples::Geant4Simulation::~Geant4Simulation() {}
+ActsExamples::Geant4Simulation::~Geant4Simulation() = default;
 
 ActsExamples::ProcessCode ActsExamples::Geant4Simulation::execute(
     const ActsExamples::AlgorithmContext& ctx) const {

Examples/Algorithms/Geant4/src/MaterialSteppingAction.cpp

@@ -23,7 +23,7 @@ ActsExamples::MaterialSteppingAction::MaterialSteppingAction(
     const Config& cfg, std::unique_ptr<const Acts::Logger> logger)
     : G4UserSteppingAction(), m_cfg(cfg), m_logger(std::move(logger)) {}
 
-ActsExamples::MaterialSteppingAction::~MaterialSteppingAction() {}
+ActsExamples::MaterialSteppingAction::~MaterialSteppingAction() = default;
 
 void ActsExamples::MaterialSteppingAction::UserSteppingAction(
     const G4Step* step) {

Examples/Algorithms/Geant4/src/SensitiveSurfaceMapper.cpp

@@ -61,7 +61,7 @@ void ActsExamples::SensitiveSurfaceMapper::remapSensitiveNames(
       // Prepare the mapped surface
       const Acts::Surface* mappedSurface = nullptr;
 
-      if (actsLayer and actsLayer->surfaceArray()) {
+      if (actsLayer != nullptr and actsLayer->surfaceArray() != nullptr) {
         auto actsSurfaces = actsLayer->surfaceArray()->at(g4AbsPosition);
         if (not actsSurfaces.empty()) {
           // Fast matching: search

Examples/Algorithms/Geant4/src/SimParticleTranslation.cpp

@@ -34,7 +34,7 @@ ActsExamples::SimParticleTranslation::SimParticleTranslation(
   }
 }
 
-ActsExamples::SimParticleTranslation::~SimParticleTranslation() {}
+ActsExamples::SimParticleTranslation::~SimParticleTranslation() = default;
 
 void ActsExamples::SimParticleTranslation::GeneratePrimaries(G4Event* anEvent) {
   anEvent->SetEventID(m_eventNr++);

Examples/Algorithms/Geant4HepMC/src/EventAction.cpp

@@ -131,7 +131,7 @@ EventAction* EventAction::instance() {
 
 EventAction::EventAction(std::vector<std::string> processFilter)
     : G4UserEventAction(), m_processFilter(std::move(processFilter)) {
-  if (s_instance) {
+  if (s_instance != nullptr) {
     throw std::logic_error("Attempted to duplicate a singleton");
   } else {
     s_instance = this;

Examples/Algorithms/Geant4HepMC/src/PrimaryGeneratorAction.cpp

@@ -27,7 +27,7 @@ PrimaryGeneratorAction::PrimaryGeneratorAction(G4int randomSeed1,
                                                G4int randomSeed2)
     : G4VUserPrimaryGeneratorAction(), m_particleGun(nullptr) {
   // Configure the run
-  if (s_instance) {
+  if (s_instance != nullptr) {
     throw std::logic_error("Attempted to duplicate a singleton");
   } else {
     s_instance = this;

Examples/Algorithms/Geant4HepMC/src/RunAction.cpp

@@ -23,7 +23,7 @@ RunAction* RunAction::instance() {
 }
 
 RunAction::RunAction() : G4UserRunAction() {
-  if (s_instance) {
+  if (s_instance != nullptr) {
     throw std::logic_error("Attempted to duplicate the RunAction singleton");
   } else {
     s_instance = this;

Examples/Algorithms/Geant4HepMC/src/SteppingAction.cpp

@@ -30,7 +30,7 @@ SteppingAction* SteppingAction::instance() {
 SteppingAction::SteppingAction(std::vector<std::string> eventRejectionProcess)
     : G4UserSteppingAction(),
       m_eventRejectionProcess(std::move(eventRejectionProcess)) {
-  if (s_instance) {
+  if (s_instance != nullptr) {
     throw std::logic_error("Attempted to duplicate a singleton");
   } else {
     s_instance = this;
@@ -114,7 +114,7 @@ void SteppingAction::UserSteppingAction(const G4Step* step) {
           vertex->add_attribute("InitialParametersOf-" + trackId, preMom4);
         }
       }
-    if (track->GetCreatorProcess())
+    if (track->GetCreatorProcess() != nullptr)
       postParticle->add_attribute(
           "CreatorProcessOf-" + trackId,
           std::make_shared<::HepMC3::StringAttribute>(

Examples/Algorithms/GeneratorsPythia8/ActsExamples/Generators/Pythia8ProcessGenerator.cpp

@@ -19,7 +19,7 @@ struct FrameworkRndmEngine : public Pythia8::RndmEngine {
   ActsExamples::RandomEngine& rng;
 
   FrameworkRndmEngine(ActsExamples::RandomEngine& rng_) : rng(rng_) {}
-  double flat() {
+  double flat() override {
     return std::uniform_real_distribution<double>(0.0, 1.0)(rng);
   }
 };
@@ -45,7 +45,7 @@ ActsExamples::Pythia8Generator::Pythia8Generator(const Config& cfg,
 }
 
 // needed to allow unique_ptr of forward-declared Pythia class
-ActsExamples::Pythia8Generator::~Pythia8Generator() {}
+ActsExamples::Pythia8Generator::~Pythia8Generator() = default;
 
 ActsExamples::SimParticleContainer ActsExamples::Pythia8Generator::operator()(
     RandomEngine& rng) {

Examples/Algorithms/HepMC/src/HepMCProcessExtractor.cpp

@@ -174,7 +174,7 @@ void filterAndSort(
 }
 }  // namespace
 
-ActsExamples::HepMCProcessExtractor::~HepMCProcessExtractor() {}
+ActsExamples::HepMCProcessExtractor::~HepMCProcessExtractor() = default;
 
 ActsExamples::HepMCProcessExtractor::HepMCProcessExtractor(
     ActsExamples::HepMCProcessExtractor::Config cfg, Acts::Logging::Level level)