Clang-Tidy II (#3190)

This is the second batch of changes fixing clang-tidy warnings.

Add CMake option AMReX_CLANG_TIDY_WERROR to update warnings to errors.

Use clang-tidy in CI: GNU@8.4 C++17 Release [lib].

.clang-tidy

@@ -3,6 +3,7 @@ Checks: >
      bugprone-*,
        -bugprone-exception-escape,
      clang-analyzer-*,
+       -clang-analyzer-optin.mpi.MPI-Checker,
      clang-diagnostic-*,
      cppcoreguidelines-*,
        -cppcoreguidelines-avoid-c-arrays,
@@ -31,11 +32,7 @@ Checks: >
        -readability-magic-numbers,
        -readability-named-parameter,
        -readability-simplify-boolean-expr,
-       -readability-convert-member-functions-to-static,
-     mpi-*,
-     openmp-*'
+     mpi-*
+    '
 
 HeaderFilterRegex: '[^n][^o][^l][^i][^n][^t]\.H$'
-
-# TODO
-# * readability-convert-member-functions-to-static

.github/workflows/gcc.yml

@@ -19,7 +19,9 @@ jobs:
     steps:
     - uses: actions/checkout@v3
     - name: Dependencies
-      run: .github/workflows/dependencies/dependencies_gcc8.sh
+      run: |
+        .github/workflows/dependencies/dependencies_gcc8.sh
+        sudo apt-get install -y --no-install-recommends clang-tidy-12
     - name: Build & Install
       run: |
         mkdir build
@@ -31,7 +33,9 @@ jobs:
             -DCMAKE_INSTALL_PREFIX=/tmp/my-amrex  \
             -DCMAKE_C_COMPILER=$(which gcc-8)     \
             -DCMAKE_CXX_COMPILER=$(which g++-8)   \
-            -DCMAKE_Fortran_COMPILER=$(which gfortran-8)
+            -DCMAKE_Fortran_COMPILER=$(which gfortran-8) \
+            -DAMReX_CLANG_TIDY=ON \
+            -DAMReX_CLANG_TIDY_WERROR=ON
         make -j 2
         make install
         make test_install

Src/Amr/AMReX_Amr.H

@@ -44,19 +44,21 @@ public:
         LevelBld* a_levelbld /* One must pass LevelBld* as an argument now*/);
 
     Amr (const Amr& rhs) = delete;
+    Amr (Amr&& rhs) = delete;
     Amr& operator= (const Amr& rhs) = delete;
+    Amr& operator= (Amr&& rhs) = delete;
 
     void InitAmr ();
 
     //! The destructor.
-    virtual ~Amr ();
+    ~Amr () override;
 
     //! Init data after construction. Must be called before timestepping.
     virtual void init (Real strt_time, Real stop_time);
 
     //! First part of initialInit
     void InitializeInit (Real strt_time, Real stop_time,
-                         const BoxArray* lev0_grids = 0, const Vector<int>* pmap = 0);
+                         const BoxArray* lev0_grids = nullptr, const Vector<int>* pmap = nullptr);
 
     //! Second part of initialInit
     void FinalizeInit (Real strt_time, Real stop_time);
@@ -68,10 +70,10 @@ public:
     void setDtLevel (Real dt, int lev) noexcept;
 
     //! Set the dtmin on each level.
-    void setDtMin (const Vector<Real>& dt_lev) noexcept;
+    void setDtMin (const Vector<Real>& dt_min_in) noexcept;
 
     //! Set the cycle count on each level.
-    void setNCycle (const Vector<int>& mss) noexcept;
+    void setNCycle (const Vector<int>& ns) noexcept;
 
     //! Subcycle in time?
     int subCycle () const noexcept { return sub_cycle; }
@@ -107,7 +109,7 @@ public:
     //! Which step are we at for the specified level?
     void setLevelCount (int lev, int n) noexcept { level_count[lev] = n; }
     //! Whether to regrid right after restart
-    bool RegridOnRestart () const noexcept;
+    static bool RegridOnRestart () noexcept;
     //! Interval between regridding.
     int regridInt (int lev) const noexcept { return regrid_int[lev]; }
     //! Number of time steps between checkpoint files.
@@ -197,7 +199,7 @@ public:
     //! More work to be done?
     int okToContinue () noexcept;
     //! Rebuild grid hierarchy finer than lbase.
-    virtual void regrid (int  lbase,
+    void regrid (int  lbase,
                          Real time,
                          bool initial = false) override;
     //! Regrid only!
@@ -208,7 +210,7 @@ public:
     static const BoxArray& initialBa (int level) noexcept
         { BL_ASSERT(level-1 < initial_ba.size()); return initial_ba[level-1]; }
     //! Number of levels at which the grids are initially specified
-    static int initialBaLevels () noexcept { return initial_ba.size(); }
+    static int initialBaLevels () noexcept { return static_cast<int>(initial_ba.size()); }
     //! Do a complete integration cycle.
     virtual void coarseTimeStep (Real stop_time);
 
@@ -226,7 +228,7 @@ public:
     //! The ith datalog file.  Do with it what you want.
     std::ostream& DataLog (int i);
     //! The filename of the ith datalog file.
-    const std::string DataLogName (int i) const noexcept { return datalogname[i]; }
+    std::string DataLogName (int i) const noexcept { return datalogname[i]; }
     //! How many datalogs have been opened
     int NumDataLogs () noexcept;
     /**
@@ -238,9 +240,9 @@ public:
     */
     static Real computeOptimalSubcycling (int   n,
                                           int*  best,
-                                          Real* dt_max,
-                                          Real* est_work,
-                                          int*  cycle_max);
+                                          const Real* dt_max,
+                                          const Real* est_work,
+                                          const int*  cycle_max);
 
     //! Write the plot file to be used for visualization.
     virtual void writePlotFile ();
@@ -252,7 +254,7 @@ public:
     virtual void checkPoint ();
     int stepOfLastCheckPoint () const noexcept {return last_checkpoint;}
 
-    const Vector<BoxArray>& getInitialBA() noexcept;
+    static const Vector<BoxArray>& getInitialBA() noexcept;
 
     /**
     * \brief Specialized version:
@@ -325,13 +327,13 @@ public:
 
     void InstallNewDistributionMap (int lev, const DistributionMapping& newdm);
 
-    bool UsingPrecreateDirectories () noexcept;
+    static bool UsingPrecreateDirectories () noexcept;
 
 protected:
 
     //! Initialize grid hierarchy -- called by Amr::init.
     void initialInit (Real strt_time, Real stop_time,
-                      const BoxArray* lev0_grids = 0, const Vector<int>* pmap = 0);
+                      const BoxArray* lev0_grids = nullptr, const Vector<int>* pmap = nullptr);
 #ifndef AMREX_NO_PROBINIT
     //! Read the probin file.
     void readProbinFile (int& init);
@@ -341,9 +343,9 @@ protected:
     //! Restart from a checkpoint file.
     void restart (const std::string& filename);
     //! Define and initialize coarsest level.
-    void defBaseLevel (Real start_time, const BoxArray* lev0_grids = 0, const Vector<int>* pmap = 0);
+    void defBaseLevel (Real strt_time, const BoxArray* lev0_grids = nullptr, const Vector<int>* pmap = nullptr);
     //! Define and initialize refined levels.
-    void bldFineLevels (Real start_time);
+    void bldFineLevels (Real strt_time);
     //! Regrid level 0 on restart.
     virtual void regrid_level_0_on_restart ();
     //! Define new grid locations (called from regrid) and put into new_grids.
@@ -355,9 +357,9 @@ protected:
     DistributionMapping makeLoadBalanceDistributionMap (int lev, Real time, const BoxArray& ba) const;
     void LoadBalanceLevel0 (Real time);
 
-    virtual void ErrorEst (int lev, TagBoxArray& tags, Real time, int ngrow) override;
-    virtual BoxArray GetAreaNotToTag (int lev) override;
-    virtual void ManualTagsPlacement (int lev, TagBoxArray& tags, const Vector<IntVect>& bf_lev) override;
+    void ErrorEst (int lev, TagBoxArray& tags, Real time, int ngrow) override;
+    BoxArray GetAreaNotToTag (int lev) override;
+    void ManualTagsPlacement (int lev, TagBoxArray& tags, const Vector<IntVect>& bf_lev) override;
 
     //! Do a single timestep on level L.
     virtual void timeStep (int  level,
@@ -367,13 +369,13 @@ protected:
                            Real stop_time);
 
     // pure virtual function in AmrCore
-    virtual void MakeNewLevelFromScratch (int /*lev*/, Real /*time*/, const BoxArray& /*ba*/, const DistributionMapping& /*dm*/) override
+    void MakeNewLevelFromScratch (int /*lev*/, Real /*time*/, const BoxArray& /*ba*/, const DistributionMapping& /*dm*/) override
         { amrex::Abort("How did we get here!"); }
-    virtual void MakeNewLevelFromCoarse (int /*lev*/, Real /*time*/, const BoxArray& /*ba*/, const DistributionMapping& /*dm*/) override
+    void MakeNewLevelFromCoarse (int /*lev*/, Real /*time*/, const BoxArray& /*ba*/, const DistributionMapping& /*dm*/) override
         { amrex::Abort("How did we get here!"); }
-    virtual void RemakeLevel (int /*lev*/, Real /*time*/, const BoxArray& /*ba*/, const DistributionMapping& /*dm*/) override
+    void RemakeLevel (int /*lev*/, Real /*time*/, const BoxArray& /*ba*/, const DistributionMapping& /*dm*/) override
         { amrex::Abort("How did we get here!"); }
-    virtual void ClearLevel (int /*lev*/) override
+    void ClearLevel (int /*lev*/) override
         { amrex::Abort("How did we get here!"); }
 
     //! Whether to write a plotfile now
@@ -395,7 +397,7 @@ protected:
     void initSubcycle();
     void initPltAndChk();
 
-    int initInSitu();
+    static int initInSitu();
     int updateInSitu();
     static int finalizeInSitu();
 

Src/Amr/AMReX_Amr.cpp

@@ -163,7 +163,7 @@ Amr::NumDataLogs () noexcept
 }
 
 bool
-Amr::RegridOnRestart () const noexcept
+Amr::RegridOnRestart () noexcept
 {
     return regrid_on_restart;
 }
@@ -204,7 +204,6 @@ Amr::derive (const std::string& name,
 
 Amr::Amr (LevelBld* a_levelbld)
     :
-    AmrCore(),
     levelbld(a_levelbld)
 {
     Initialize();
@@ -513,7 +512,7 @@ Amr::initInSitu()
 }
 
 int
-Amr::updateInSitu()
+Amr::updateInSitu() // NOLINT(readability-convert-member-functions-to-static)
 {
 #if defined(AMREX_USE_SENSEI_INSITU) && !defined(AMREX_NO_SENSEI_AMR_INST)
     if (insitu_bridge && insitu_bridge->update(this))
@@ -841,7 +840,7 @@ Amr::writePlotFile ()
 
     // Don't continue if we have no variables to plot.
 
-    if (statePlotVars().size() == 0) {
+    if (statePlotVars().empty()) {
         return;
     }
 
@@ -880,7 +879,7 @@ Amr::writeSmallPlotFile ()
 
     // Don't continue if we have no variables to plot.
 
-    if (stateSmallPlotVars().size() == 0 && deriveSmallPlotVars().size() == 0) {
+    if (stateSmallPlotVars().empty() && deriveSmallPlotVars().empty()) {
         return;
     }
 
@@ -1396,14 +1395,15 @@ Amr::restart (const std::string& filename)
       bool bExitOnError(false);  // ---- dont exit if this file does not exist
       ParallelDescriptor::ReadAndBcastFile(faHeaderFilesName, faHeaderFileChars,
                                            bExitOnError);
-      if(faHeaderFileChars.size() > 0) {  // ---- headers were read
+      if(!faHeaderFileChars.empty()) {  // ---- headers were read
         std::string faFileCharPtrString(faHeaderFileChars.dataPtr());
         std::istringstream fais(faFileCharPtrString, std::istringstream::in);
         while ( ! fais.eof()) {  // ---- read and broadcast each header
           std::string faHeaderName;
           fais >> faHeaderName;
           if( ! fais.eof()) {
-              std::string faHeaderFullName(filename + '/' + faHeaderName + "_H");
+              std::string faHeaderFullName(filename);
+              faHeaderFullName.append("/").append(faHeaderName).append("_H");
               Vector<char> &tempCharArray = faHeaderMap[faHeaderFullName];
               ParallelDescriptor::ReadAndBcastFile(faHeaderFullName, tempCharArray);
               if(verbose > 2) {
@@ -1796,7 +1796,7 @@ Amr::checkPoint ()
 
     if (ParallelDescriptor::IOProcessor()) {
         const Vector<std::string> &FAHeaderNames = StateData::FabArrayHeaderNames();
-        if(FAHeaderNames.size() > 0) {
+        if(!FAHeaderNames.empty()) {
             std::string FAHeaderFilesName = ckfileTemp + "/FabArrayHeaders.txt";
             std::ofstream FAHeaderFile(FAHeaderFilesName.c_str(),
                                        std::ios::out | std::ios::trunc |
@@ -2475,7 +2475,7 @@ Amr::defBaseLevel (Real              strt_time,
 
     BoxArray lev0;
 
-    if (lev0_grids != nullptr && lev0_grids->size() > 0)
+    if (lev0_grids != nullptr && !lev0_grids->empty())
     {
         BL_ASSERT(pmap != nullptr);
 
@@ -3245,7 +3245,7 @@ Amr::okToRegrid(int level) noexcept
 }
 
 Real
-Amr::computeOptimalSubcycling(int n, int* best, Real* dt_max, Real* est_work, int* cycle_max)
+Amr::computeOptimalSubcycling(int n, int* best, const Real* dt_max, const Real* est_work, const int* cycle_max)
 {
     BL_ASSERT(cycle_max[0] == 1);
     // internally these represent the total number of steps at a level,