Require clang-tidy in CI

.clang-tidy

@@ -23,10 +23,12 @@ Checks: |
   -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-avoid-non-const-global-variables.AllowInternalLinkage: true
 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,18 +186,19 @@ 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
       - name: Check installation
-        if: ${{steps.install.outcome == 'success'}}
+        if: ${{steps.install.outcome == 'success' && matrix.special != 'clang-tidy'}}
         working-directory: install
         run: |
           for exe in orange-update celer-export-geant celer-dump-data \

app/celer-dump-data.cc

@@ -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/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_))
     {

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/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

@@ -54,6 +54,46 @@ enum class OpticalProcessType
     size_,
 };
 
+/*!
+ * Wrapper around a unique pointer to accomodate keeping track whether we
+ * deleguated ownership to Geant4. We have to assume that Geant4 won't free the
+ * memory before we're done reading it...
+ */
+template<class T>
+class UniquePtrOwner
+{
+  public:
+    explicit UniquePtrOwner(std::unique_ptr<T> ptr) : ptr_(std::move(ptr)) {};
+    UniquePtrOwner(UniquePtrOwner const&) = delete;
+    UniquePtrOwner& operator=(UniquePtrOwner const&) = delete;
+    UniquePtrOwner(UniquePtrOwner&&) noexcept = default;
+    UniquePtrOwner& operator=(UniquePtrOwner&&) noexcept = default;
+
+    T* transferOwnership() noexcept
+    {
+        is_owner_ = false;
+        return ptr_.get();
+    };
+
+    operator T*() const noexcept { return ptr_.get(); };
+
+    T* operator->() const noexcept { return ptr_.get(); };
+
+    ~UniquePtrOwner()
+    {
+        // we transfered ownership, don't free the memory
+        if (!is_owner_)
+        {
+            // NOLINTNEXTLINE(bugprone-unused-return-value)
+            ptr_.release();
+        }
+    };
+
+  private:
+    std::unique_ptr<T> ptr_;
+    bool is_owner_{true};
+};
+
 #if G4VERSION_NUMBER >= 1070
 G4OpticalProcessIndex
 optical_process_type_to_geant_index(OpticalProcessType value)
@@ -239,13 +279,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 = UniquePtrOwner{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.transferOwnership());
         CELER_LOG(debug)
             << "Loaded Optical boundary process with G4OpBoundaryProcess "
                "process";
@@ -275,7 +315,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 = UniquePtrOwner{std::make_unique<G4Scintillation>()};
 #if G4VERSION_NUMBER < 1070
     scint->SetStackPhotons(options_.scintillation.stack_photons);
     scint->SetTrackSecondariesFirst(
@@ -290,7 +330,7 @@ void CelerOpticalPhysics::ConstructProcess()
 #endif
     scint->AddSaturation(G4LossTableManager::Instance()->EmSaturation());
 
-    auto cerenkov = std::make_unique<G4Cerenkov>();
+    auto cerenkov = UniquePtrOwner{std::make_unique<G4Cerenkov>()};
 #if G4VERSION_NUMBER < 1070
     cerenkov->SetStackPhotons(options_.cerenkov.stack_photons);
     cerenkov->SetTrackSecondariesFirst(
@@ -311,18 +351,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.transferOwnership());
+            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.transferOwnership());
+            process_manager->SetProcessOrderingToLast(scint, idxAtRest);
+            process_manager->SetProcessOrderingToLast(scint, idxPostStep);
             CELER_LOG(debug)
                 << "Loaded Optical Scintillation with G4Scintillation "
                    "process for particle "
@@ -331,13 +371,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/geo/GeoMaterialParams.cc

@@ -40,9 +40,11 @@ using MapLabelMatId = std::unordered_map<Label, MaterialId>;
  * The input is effectively an "unzipped" unordered list of (volume label,
  * material id) pairs.
  */
-MapLabelMatId build_label_map(MaterialParams const& mat_params,
-                              std::vector<Label>&& labels,
-                              std::vector<MaterialId> const& materials)
+MapLabelMatId build_label_map(
+    MaterialParams const& mat_params,
+    std::vector<Label>&&
+        labels,  // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved)
+    std::vector<MaterialId> const& materials)
 {
     CELER_EXPECT(materials.size() == labels.size());
     CELER_EXPECT(std::all_of(

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/optical/OpticalCollector.cc

@@ -34,6 +34,7 @@ namespace celeritas
  *
  * This adds several actions and auxiliary data to the registry.
  */
+// NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved)
 OpticalCollector::OpticalCollector(CoreParams const& core, Input&& inp)
 {
     CELER_EXPECT(inp);

src/celeritas/optical/detail/OpticalLaunchAction.cc

@@ -48,10 +48,11 @@ OpticalLaunchAction::make_and_insert(CoreParams const& core, Input&& input)
 /*!
  * Construct with action ID, generator storage.
  */
-OpticalLaunchAction::OpticalLaunchAction(ActionId action_id,
-                                         AuxId data_id,
-                                         CoreParams const& core,
-                                         Input&& input)
+OpticalLaunchAction::OpticalLaunchAction(
+    ActionId action_id,
+    AuxId data_id,
+    CoreParams const& core,
+    Input&& input)  // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved)
     : action_id_{action_id}
     , aux_id_{data_id}
     , offload_params_{std::move(input.offload)}

src/celeritas/user/SlotDiagnostic.cc

@@ -32,7 +32,16 @@ struct SlotDiagnostic::State final : AuxStateInterface
     std::ofstream outfile;
     std::vector<int> buffer;
 
-    ~State() { CELER_LOG_LOCAL(debug) << "Closing slot diagnostic file"; }
+    State() = default;
+    State(State const&) = delete;
+    State& operator=(State const&) = delete;
+    State(State&&) noexcept = default;
+    State& operator=(State&&) noexcept = default;
+
+    ~State() final
+    {
+        CELER_LOG_LOCAL(debug) << "Closing slot diagnostic file";
+    }
 };
 
 //---------------------------------------------------------------------------//

src/corecel/data/CollectionMirror.hh

@@ -48,7 +48,7 @@ namespace celeritas
  * \endcode
  */
 template<template<Ownership, MemSpace> class P>
-class CollectionMirror : public ParamsDataInterface<P>
+class CollectionMirror final : public ParamsDataInterface<P>
 {
   public:
     //!@{

src/corecel/io/ColorUtils.cc

@@ -86,6 +86,7 @@ char const* color_code(char abbrev)
     if (!use_color())
         return "";
 
+    // NOLINTNEXTLINE(bugprone-switch-missing-default-case)
     switch (abbrev)
     {
         case 'g':

src/corecel/io/ExceptionOutput.cc

@@ -84,6 +84,7 @@ void eptr_to_json(nlohmann::json& j, std::exception_ptr const& eptr)
     {
         try_nested_to_json(j, e);
     }
+    // NOLINTNEXTLINE(bugprone-empty-catch)
     catch (...)
     {
     }

src/corecel/io/LoggerTypes.cc

@@ -40,7 +40,7 @@ char const* to_color_code(LogLevel lev)
     char c = ' ';
     switch (lev)
     {
-        case LogLevel::debug:      c = 'x'; break;
+        case LogLevel::debug:
         case LogLevel::diagnostic: c = 'x'; break;
         case LogLevel::status:     c = 'b'; break;
         case LogLevel::info:       c = 'g'; break;

src/corecel/io/detail/ReprImpl.cc

@@ -68,6 +68,7 @@ void repr_char(std::ostream& os, char value)
 std::string char_to_hex_string(unsigned char value)
 {
     char buffer[3];
+    // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
     int size = std::snprintf(buffer, sizeof(buffer), "%02hhx", value);
     CELER_ENSURE(size == 2);
     return {buffer, buffer + 2};