Require clang-tidy in CI (#1524)

.clang-tidy

@@ -1,9 +1,14 @@
 ---
 Checks: |
   -*
-  clang-analyzer-cplusplus.Move
-  clang-analyzer-cplusplus.NewDeleteLeaks
-  clang-analyzer-core.StackAddressEscape
+  clang-analyzer-core.*
+  clang-analyzer-deadcode.*
+  clang-analyzer-cplusplus.*
+  -clang-analyzer-cplusplus.NewDelete
+  performance-*
+  -performance-avoid-endl
+  -performance-unnecessary-value-param
+  -performance-enum-size
   cppcoreguidelines-*
   -cppcoreguidelines-owning-memory
   -cppcoreguidelines-narrowing-conversions
@@ -17,16 +22,24 @@ Checks: |
   -cppcoreguidelines-pro-bounds-array-to-pointer-decay
   -cppcoreguidelines-avoid-const-or-ref-data-members
   -cppcoreguidelines-pro-bounds-constant-array-index
+  -cppcoreguidelines-noexcept-swap
+  -cppcoreguidelines-noexcept-move-operations
+  -cppcoreguidelines-noexcept-destructor
   bugprone-*
   -bugprone-sizeof-expression
   -bugprone-narrowing-conversions
   -bugprone-macro-parentheses
   -bugprone-easily-swappable-parameters
   -bugprone-implicit-widening-of-multiplication-result
+WarningsAsErrors: '*'
 CheckOptions:
   cppcoreguidelines-macro-usage.CheckCapsOnly: true
   cppcoreguidelines-avoid-do-while.IgnoreMacros: true
   cppcoreguidelines-rvalue-reference-param-not-moved.IgnoreNonDeducedTemplateTypes: true
+  cppcoreguidelines-rvalue-reference-param-not-moved.AllowPartialMove: true
+  cppcoreguidelines-avoid-non-const-global-variables.AllowInternalLinkage: true
+  performance-move-const-arg.CheckTriviallyCopyableMove: false
+  performance-move-const-arg.CheckMoveToConstRef: false
 HeaderFilterRegex: ''
 FormatStyle: file
 ...

.github/workflows/build-spack.yml

@@ -41,7 +41,7 @@ jobs:
           - geometry: "geant4"
             special: null
             geant: "11.0"
-          - geometry: "orange"
+          - geometry: "vecgeom"
             special: "clang-tidy"
             geant: "11.2"
     env:
@@ -56,6 +56,7 @@ jobs:
       SPACK_BUILDCACHE: "celer-buildcache" # see spack.yaml
       CC: "clang-15"
       CXX: "clang++-15"
+      CLANG_TIDY: "clang-tidy-15"
     runs-on: ubuntu-22.04
     continue-on-error: false
     steps:
@@ -126,8 +127,6 @@ jobs:
         run: |
           ccache -z
       - name: Configure Celeritas
-        env:
-          CLANG_TIDY: "clang-tidy-15"
         run: |
           ln -fs scripts/cmake-presets/ci-ubuntu-github.json CMakeUserPresets.json
           if [ "${{matrix.geant}}" == "11.0" ]; then
@@ -139,9 +138,12 @@ jobs:
       - name: Build all
         id: build
         working-directory: build
-        continue-on-error: ${{matrix.special == 'clang-tidy'}}
         run: |
-          ninja -v -k0
+          if [ "${{matrix.special}}" == "clang-tidy" ]; then
+            ninja -k0
+          else
+            ninja -v -k0
+          fi
       - name: Regenerate ROOT test data
         if: ${{matrix.geant == '11.0'}}
         working-directory: build
@@ -184,13 +186,14 @@ jobs:
         if: >-
           ${{
             (matrix.special != 'asanlite')
+            && (matrix.special != 'clang-tidy')
           }}
         run: |
           . ${SPACK_VIEW}/rc
           CELER_INSTALL_DIR=${PWD}/build ./scripts/ci/test-examples.sh
       - name: Install
         id: install
-        if: ${{!cancelled() && steps.build.outcome == 'success'}}
+        if: ${{!cancelled() && steps.build.outcome == 'success' && matrix.special != 'clang-tidy'}}
         working-directory: build
         run: |
           ninja install

app/celer-dump-data.cc

@@ -624,7 +624,7 @@ void print_trans_params(ImportTransParameters const& trans_params,
     for (auto const& kv : trans_params.looping)
     {
         auto pid = particles.find(PDGNumber{kv.first});
-        auto par = particles.id_to_label(pid);
+        auto const& par = particles.id_to_label(pid);
         cout << PEP_STREAM_PAR_PARAM(threshold_trials, par)
              << PEP_STREAM_PAR_PARAM(important_energy, par);
     }
@@ -810,8 +810,8 @@ class OpticalMfpHelper
     };
 
     //! Construct helper for given model class out of the models
-    OpticalMfpHelper(std::vector<ImportOpticalModel> const& models, optical::ImportModelClass imc)
-        : mfps_(nullptr)
+    OpticalMfpHelper(std::vector<ImportOpticalModel> const& models,
+                     optical::ImportModelClass imc)
     {
         auto iter = std::find_if(models.begin(), models.end(), [imc] (auto const& m) { return m.model_class == imc; });
         if (iter != models.end())
@@ -831,7 +831,7 @@ class OpticalMfpHelper
     }
 
   private:
-    std::vector<ImportPhysicsVector> const* mfps_;
+    std::vector<ImportPhysicsVector> const* mfps_{nullptr};
 };
 
 /*!
@@ -968,7 +968,7 @@ void print_optical_materials(std::vector<ImportOpticalModel> const& io_models,
 /*!
  * Execute and run.
  */
-int main(int argc, char* argv[])
+int main(int argc, char* argv[])  // NOLINT(bugprone-exception-escape)
 {
     using namespace celeritas;
     using namespace celeritas::app;

app/celer-g4/DetectorConstruction.cc

@@ -312,6 +312,7 @@ void DetectorConstruction::ConstructSDandField()
  * Apply a function to the range of volumes for each detector.
  */
 template<class F>
+// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward)
 void DetectorConstruction::foreach_detector(F&& apply_to_range) const
 {
     auto start = detectors_.begin();

app/celer-g4/GeantDiagnostics.cc

@@ -75,7 +75,7 @@ GeantDiagnostics::GeantDiagnostics(SharedParams const& params)
     CELER_LOG_LOCAL(status) << "Initializing Geant4 diagnostics";
 
     // Get output registry
-    auto output_reg = params.output_reg();
+    auto const& output_reg = params.output_reg();
     CELER_ASSERT(output_reg);
     size_type num_threads = params.num_streams();
 

app/celer-g4/SensitiveHit.cc

@@ -24,6 +24,7 @@ namespace app
  */
 auto SensitiveHit::allocator() -> HitAllocator&
 {
+    // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
     static G4ThreadLocal HitAllocator* alloc_;
     if (CELER_UNLIKELY(!alloc_))
     {
@@ -36,10 +37,7 @@ auto SensitiveHit::allocator() -> HitAllocator&
 /*!
  * Construct with hit data.
  */
-SensitiveHit::SensitiveHit(EventHitData const& hit)
-    : G4VHit(), data_{std::move(hit)}
-{
-}
+SensitiveHit::SensitiveHit(EventHitData const& hit) : G4VHit(), data_{hit} {}
 
 //---------------------------------------------------------------------------//
 }  // namespace app

app/celer-geo/celer-geo.cc

@@ -132,6 +132,7 @@ void run_trace(Runner& run_trace,
         std::ofstream image_file(trace_setup.bin_file, std::ios::binary);
         std::vector<int> image_data(img_params.num_pixels());
         image->copy_to_host(make_span(image_data));
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
         image_file.write(reinterpret_cast<char const*>(image_data.data()),
                          image_data.size() * sizeof(int));
     }

scripts/cmake-presets/ci-ubuntu-github.json

@@ -77,9 +77,9 @@
       "inherits": ["spack"]
     },
     {
-      "name": "reldeb-orange-clang-tidy",
+      "name": "reldeb-vecgeom-clang-tidy",
       "description": "Build with ORANGE and clang-tidy checks",
-      "inherits": ["spack"],
+      "inherits": ["spack", ".vecgeom"],
       "cacheVariables": {
         "CELERITAS_DEBUG":       {"type": "BOOL", "value": "ON"},
         "CELERITAS_BUILD_TESTS": {"type": "BOOL", "value": "OFF"},

src/accel/ExceptionConverter.cc

@@ -166,7 +166,7 @@ void ExceptionConverter::operator()(std::exception_ptr eptr) const
                     msg << "\n[error while exporting state: " << e.what()
                         << "]";
                 }
-                catch (...)
+                catch (...)  // NOLINT(bugprone-empty-catch)
                 {
                     /* Do nothing */
                 }

src/accel/SharedParams.cc

@@ -599,7 +599,7 @@ void SharedParams::initialize_core(SetupOptions const& options)
         asfi.cutoff = params.cutoff;
         asfi.physics = params.physics;
         asfi.imported = imported;
-        auto const along_step{options.make_along_step(asfi)};
+        auto along_step{options.make_along_step(asfi)};
         CELER_VALIDATE(along_step,
                        << "along-step factory returned a null pointer");
         return along_step;

src/accel/detail/HitProcessor.cc

@@ -141,7 +141,7 @@ HitProcessor::~HitProcessor()
     {
         CELER_LOG_LOCAL(debug) << "Deallocating hit processor";
     }
-    catch (...)
+    catch (...)  // NOLINT(bugprone-empty-catch)
     {
         // Ignore anything bad that happens while logging
     }

src/celeritas/ext/detail/CelerOpticalPhysics.cc

@@ -20,6 +20,8 @@
 #include <G4ProcessManager.hh>
 #include <G4Scintillation.hh>
 #include <G4Version.hh>
+
+#include "corecel/Macros.hh"
 #if G4VERSION_NUMBER >= 1070
 #    include <G4OpWLS2.hh>
 #    include <G4OpticalParameters.hh>
@@ -54,6 +56,33 @@ enum class OpticalProcessType
     size_,
 };
 
+/*!
+ * Wrapper around a unique pointer to accomodate keeping track whether we
+ * delegated ownership to Geant4. We have to assume that Geant4 won't free the
+ * memory before we're done reading it...
+ */
+template<class T>
+class ObservingUniquePtr
+{
+  public:
+    explicit ObservingUniquePtr(std::unique_ptr<T> ptr)
+        : uptr_(std::move(ptr)), ptr_{uptr_.get()}
+    {
+    }
+    // False positive(fixed in clang-tidy-18)
+    // NOLINTNEXTLINE(performance-noexcept-move-constructor)
+    CELER_DEFAULT_MOVE_DELETE_COPY(ObservingUniquePtr);
+    ~ObservingUniquePtr() = default;
+
+    T* release() noexcept { return uptr_.release(); }
+    operator T*() const noexcept { return ptr_; }
+    T* operator->() const noexcept { return ptr_; }
+
+  private:
+    std::unique_ptr<T> uptr_;
+    T* ptr_;
+};
+
 #if G4VERSION_NUMBER >= 1070
 G4OpticalProcessIndex
 optical_process_type_to_geant_index(OpticalProcessType value)
@@ -239,13 +268,13 @@ void CelerOpticalPhysics::ConstructProcess()
 
     // NB: boundary is also used later on in loop over particles,
     // though it's only ever applicable to G4OpticalPhotons
-    auto boundary = std::make_unique<G4OpBoundaryProcess>();
+    auto boundary = ObservingUniquePtr{std::make_unique<G4OpBoundaryProcess>()};
 #if G4VERSION_NUMBER < 1070
     boundary->SetInvokeSD(options_.boundary.invoke_sd);
 #endif
     if (process_is_active(OpticalProcessType::boundary, options_))
     {
-        process_manager->AddDiscreteProcess(boundary.get());
+        process_manager->AddDiscreteProcess(boundary.release());
         CELER_LOG(debug)
             << "Loaded Optical boundary process with G4OpBoundaryProcess "
                "process";
@@ -275,7 +304,7 @@ void CelerOpticalPhysics::ConstructProcess()
 
     // Add photon-generating processes to all particles they apply to
     // TODO: Eventually replace with Celeritas step collector processes
-    auto scint = std::make_unique<G4Scintillation>();
+    auto scint = ObservingUniquePtr{std::make_unique<G4Scintillation>()};
 #if G4VERSION_NUMBER < 1070
     scint->SetStackPhotons(options_.scintillation.stack_photons);
     scint->SetTrackSecondariesFirst(
@@ -290,7 +319,7 @@ void CelerOpticalPhysics::ConstructProcess()
 #endif
     scint->AddSaturation(G4LossTableManager::Instance()->EmSaturation());
 
-    auto cerenkov = std::make_unique<G4Cerenkov>();
+    auto cerenkov = ObservingUniquePtr{std::make_unique<G4Cerenkov>()};
 #if G4VERSION_NUMBER < 1070
     cerenkov->SetStackPhotons(options_.cerenkov.stack_photons);
     cerenkov->SetTrackSecondariesFirst(
@@ -311,18 +340,18 @@ void CelerOpticalPhysics::ConstructProcess()
         if (cerenkov->IsApplicable(*p)
             && process_is_active(OpticalProcessType::cerenkov, options_))
         {
-            process_manager->AddProcess(cerenkov.get());
-            process_manager->SetProcessOrdering(cerenkov.get(), idxPostStep);
+            process_manager->AddProcess(cerenkov.release());
+            process_manager->SetProcessOrdering(cerenkov, idxPostStep);
             CELER_LOG(debug) << "Loaded Optical Cerenkov with G4Cerenkov "
                                 "process for particle "
                              << p->GetParticleName();
         }
         if (scint->IsApplicable(*p)
             && process_is_active(OpticalProcessType::scintillation, options_))
         {
-            process_manager->AddProcess(scint.get());
-            process_manager->SetProcessOrderingToLast(scint.get(), idxAtRest);
-            process_manager->SetProcessOrderingToLast(scint.get(), idxPostStep);
+            process_manager->AddProcess(scint.release());
+            process_manager->SetProcessOrderingToLast(scint, idxAtRest);
+            process_manager->SetProcessOrderingToLast(scint, idxPostStep);
             CELER_LOG(debug)
                 << "Loaded Optical Scintillation with G4Scintillation "
                    "process for particle "
@@ -331,13 +360,9 @@ void CelerOpticalPhysics::ConstructProcess()
         if (boundary->IsApplicable(*p)
             && process_is_active(OpticalProcessType::boundary, options_))
         {
-            process_manager->SetProcessOrderingToLast(boundary.get(),
-                                                      idxPostStep);
+            process_manager->SetProcessOrderingToLast(boundary, idxPostStep);
         }
     }
-    boundary.release();
-    cerenkov.release();
-    scint.release();
 }
 
 }  // namespace detail

src/celeritas/global/CoreState.cc

@@ -83,7 +83,7 @@ CoreState<M>::~CoreState()
             << "Deallocating " << to_cstring(M) << " core state (stream "
             << this->stream_id().unchecked_get() << ')';
     }
-    catch (...)
+    catch (...)  // NOLINT(bugprone-empty-catch)
     {
         // Ignore anything bad that happens while logging
     }

src/celeritas/mat/MaterialParams.cc

@@ -349,6 +349,7 @@ void MaterialParams::append_element_def(ElementInput const& inp,
 
     // Isotopic data
     std::vector<ElIsotopeComponent> vec_eic;
+    vec_eic.reserve(inp.isotopes_fractions.size());
     for (auto const& key : inp.isotopes_fractions)
     {
         vec_eic.push_back(ElIsotopeComponent{key.first, key.second});

src/celeritas/phys/ImportedProcessAdapter.cc

@@ -203,7 +203,7 @@ auto ImportedProcessAdapter::step_limits(Applicability const& applic) const
 {
     try
     {
-        return this->step_limits_impl(std::move(applic));
+        return this->step_limits_impl(applic);
     }
     catch (...)
     {

src/celeritas/phys/PrimaryGenerator.cc

@@ -33,7 +33,7 @@ PrimaryGenerator::from_options(SPConstParticles particles,
 
     PrimaryGenerator::Input inp;
     inp.seed = opts.seed;
-    inp.pdg = std::move(opts.pdg);
+    inp.pdg = opts.pdg;
     inp.num_events = opts.num_events;
     inp.primaries_per_event = opts.primaries_per_event;
     inp.sample_energy = make_energy_sampler(opts.energy);
@@ -49,9 +49,9 @@ PrimaryGenerator::from_options(SPConstParticles particles,
 PrimaryGenerator::PrimaryGenerator(SPConstParticles particles, Input const& inp)
     : num_events_(inp.num_events)
     , primaries_per_event_(inp.primaries_per_event)
-    , sample_energy_(std::move(inp.sample_energy))
-    , sample_pos_(std::move(inp.sample_pos))
-    , sample_dir_(std::move(inp.sample_dir))
+    , sample_energy_(inp.sample_energy)
+    , sample_pos_(inp.sample_pos)
+    , sample_dir_(inp.sample_dir)
 {
     CELER_EXPECT(particles);
 

src/celeritas/user/SimpleCalo.cc

@@ -128,6 +128,7 @@ void SimpleCalo::output(JsonPimpl* j) const
     // Save detector volumes
     {
         std::vector<int> ids;
+        ids.reserve(volume_ids_.size());
         for (VolumeId vid : volume_ids_)
         {
             ids.push_back(static_cast<int>(vid.get()));