Clang-tidy CI test: bump version from 16 to 17 (#5600)

This PR bumps the version used for `clang-tidy` CI tests from 16 to 17.
It also addresses all the issues found with the upgraded tool.

To be merged **after** #5592 ✅

### The issues found 🧐 and fixed 🛠️ with the upgraded tool are the
following :

-
[bugprone-switch-missing-default-case](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.html)
A newly introduced check to flag `switch` statements without a `default`
case (unless the argument is an `enum`)

-
[cppcoreguidelines-rvalue-reference-param-not-moved](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.html)
A newly introduced check to flag when an rvalue reference argument of a
function is never moved inside the function body.
⚠️ **Warning**: in order to have this check compatible with
[performance-move-const-arg](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/performance/move-const-arg.html)
I had to set `performance-move-const-arg.CheckTriviallyCopyableMove` to
`false` (specifically for the three methods in
`ablastr::utils::msg_logger` accepting
`std::vector<char>::const_iterator&& rit` arguments).

-
[misc-header-include-cycle](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/misc/header-include-cycle.html)
A newly introduced check to prevent cyclic header inclusions.

-
[modernize-type-traits](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/modernize/type-traits.html)
A newly introduced check. The idea is to replace currencies of, e.g.,
`std::is_integral<T>::value`, with the less verbose alternative
`std::is_integral_v<T>`

-
[performance-avoid-endl](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/performance/avoid-endl.html)
A newly introduced check. The idea is to replace `<< std::endl` with
`\n`, since `endl` also forces a flush of the stream. In few cases
flushing the buffer is actually the desired behavior. Typically, this
happens when we want to write to `std::cerr`, which is however
automatically flushed after each write operation. In cases where
actually flushing to `std::cout` is the desired behavior one can do `<<
\n << std::flush `, which is arguably more transparent than `<<
std::endl`.

-
[performance-noexcept-swap](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/performance/noexcept-swap.html)
For performance reasons it is better if `swap` functions are declared as
`noexcept`, in order to allow the compiler to perform more aggressive
optimizations. In any case, we can use the AMReX function `amrex::Swap`,
which is `noexcept`.

### 🔄 Re-enabled checks:

-
[readability-misleading-indentation](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/readability/misleading-indentation.html)
This check was already available in v16, but a bug led to false
positives. The bug has been corrected in v17 of the tool, so we can
re-enable the check.

### ⛔ The PR excludes the following checks :

-
[cppcoreguidelines-missing-std-forward](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.html)
A newly introduced check that warns when a forwarding reference
parameter is not forwarded. In order to comply with this check I think
that I have to pass some parameters by reference to lambda functions
inside `ParallelFor` constructs. However, this leads to issues when we
compile for GPUs. Therefore, I think that the best solution is to
exclude this check. See an example below (for `PredFunc&& filter` ):
```
amrex::ParallelForRNG(np,
    [=,&filter] AMREX_GPU_DEVICE (int i, amrex::RandomEngine const& engine)
    {
        p_mask[i] = filter(src_data, i, engine);
    });
``` 

-
[misc-include-cleaner](https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/misc/include-cleaner.html)
It would be awesome to include this check. However, as it is now
implemented, it has no notion of "associated headers". For instance,
let's suppose that the header `MyClass.H` has `#include<string>` and
that `MyClass.cpp` has `#include "MyClass.H"` and uses `std:string`
somewhere. In this case, the check raises a warning stating that you
should include `<string>` in `MyClass.cpp` even if it is transitively
included via the associate header `MyClass.H` . For this reason, for the
moment, it is better to periodically check headers with the `IWYU` tool.

.clang-tidy

@@ -19,6 +19,7 @@ Checks: '
         -cppcoreguidelines-avoid-non-const-global-variables,
         -cppcoreguidelines-init-variables,
         -cppcoreguidelines-macro-usage,
+        -cppcoreguidelines-missing-std-forward,
         -cppcoreguidelines-narrowing-conversions,
         -cppcoreguidelines-non-private-member-variables-in-classes,
         -cppcoreguidelines-owning-memory,
@@ -29,6 +30,7 @@ Checks: '
     misc-*,
         -misc-no-recursion,
         -misc-non-private-member-variables-in-classes,
+        -misc-include-cleaner,
     modernize-*,
         -modernize-avoid-c-arrays,
         -modernize-return-braced-init-list,
@@ -44,7 +46,6 @@ Checks: '
         -readability-implicit-bool-conversion,
         -readability-isolate-declaration,
         -readability-magic-numbers,
-        -readability-misleading-indentation,
         -readability-named-parameter,
         -readability-uppercase-literal-suffix
     '
@@ -58,6 +59,7 @@ CheckOptions:
   value:        "true"
 - key:          misc-use-anonymous-namespace.HeaderFileExtensions
   value:        "H,"
-
+- key:          performance-move-const-arg.CheckTriviallyCopyableMove
+  value:        "false"
 
 HeaderFilterRegex: 'Source[a-z_A-Z0-9\/]+\.H$'

.github/workflows/clang_tidy.yml

@@ -26,7 +26,7 @@ jobs:
     - uses: actions/checkout@v4
     - name: install dependencies
       run: |
-        .github/workflows/dependencies/clang.sh 16
+        .github/workflows/dependencies/clang.sh 17
     - name: set up cache
       uses: actions/cache@v4
       with:
@@ -43,8 +43,8 @@ jobs:
         export CCACHE_LOGFILE=${{ github.workspace }}/ccache.log.txt
         ccache -z
 
-        export CXX=$(which clang++-16)
-        export CC=$(which clang-16)
+        export CXX=$(which clang++-17)
+        export CC=$(which clang-17)
 
         cmake -S . -B build_clang_tidy      \
           -DCMAKE_VERBOSE_MAKEFILE=ON       \
@@ -62,7 +62,7 @@ jobs:
 
         ${{github.workspace}}/.github/workflows/source/makeMakefileForClangTidy.py --input ${{github.workspace}}/ccache.log.txt
         make -j4 --keep-going -f clang-tidy-ccache-misses.mak \
-            CLANG_TIDY=clang-tidy-16 \
+            CLANG_TIDY=clang-tidy-17 \
             CLANG_TIDY_ARGS="--config-file=${{github.workspace}}/.clang-tidy --warnings-as-errors=*"
 
         ccache -s

Source/Diagnostics/FlushFormats/FlushFormatCatalyst.cpp

@@ -110,7 +110,7 @@ FlushFormatCatalyst::FlushFormatCatalyst() {
     if (err != catalyst_status_ok)
     {
         std::string message = " Error: Failed to initialize Catalyst!\n";
-        std::cerr << message << err << std::endl;
+        std::cerr << message << err << "\n";
         amrex::Print() << message;
         amrex::Abort(message);
     }
@@ -180,7 +180,7 @@ FlushFormatCatalyst::WriteToFile (
     if (err != catalyst_status_ok)
     {
         std::string message = " Error: Failed to execute Catalyst!\n";
-        std::cerr << message << err << std::endl;
+        std::cerr << message << err << "\n";
         amrex::Print() << message;
     }
     WARPX_PROFILE_VAR_STOP(prof_catalyst_execute);
@@ -200,7 +200,7 @@ FlushFormatCatalyst::~FlushFormatCatalyst() {
     if (err != catalyst_status_ok)
     {
         std::string message = " Error: Failed to finalize Catalyst!\n";
-        std::cerr << message << err << std::endl;
+        std::cerr << message << err << "\n";
         amrex::Print() << message;
         amrex::Abort(message);
     } else {

Source/Diagnostics/FullDiagnostics.cpp

@@ -873,7 +873,7 @@ FullDiagnostics::InitializeFieldFunctors (int lev)
         } else if ( m_varnames[comp] == "divE" ){
             m_all_field_functors[lev][comp] = std::make_unique<DivEFunctor>(warpx.m_fields.get_alldirs(FieldType::Efield_aux, lev), lev, m_crse_ratio);
         } else {
-            std::cout << "Error on component " << m_varnames[comp] << std::endl;
+            std::cout << "Error on component " << m_varnames[comp] << "\n";
             WARPX_ABORT_WITH_MESSAGE(m_varnames[comp] + " is not a known field output type for this geometry");
         }
     }

Source/Diagnostics/ReducedDiags/Timestep.cpp

@@ -50,7 +50,7 @@ Timestep::Timestep (const std::string& rd_name)
         }
 
         // close file
-        ofs << std::endl;
+        ofs << "\n";
         ofs.close();
     }
 }

Source/FieldSolver/MagnetostaticSolver/MagnetostaticSolver.cpp

@@ -130,7 +130,7 @@ WarpX::AddMagnetostaticFieldLabFrame()
     // temporary fix!!!
     const amrex::Real absolute_tolerance = 0.0;
     amrex::Real required_precision;
-    if constexpr (std::is_same<Real, float>::value) {
+    if constexpr (std::is_same_v<Real, float>) {
         required_precision = 1e-5;
     }
     else {

Source/FieldSolver/SpectralSolver/SpectralKSpace.H

@@ -17,6 +17,7 @@
 #include <AMReX_Array.H>
 #include <AMReX_BoxArray.H>
 #include <AMReX_Config.H>
+#include <AMReX_Enum.H>
 #include <AMReX_GpuContainers.H>
 #include <AMReX_LayoutData.H>
 #include <AMReX_REAL.H>
@@ -35,9 +36,9 @@ using SpectralShiftFactor = amrex::LayoutData<
 
 // Indicate the type of correction "shift" factor to apply
 // when the FFT is performed from/to a cell-centered grid in real space.
-struct ShiftType {
-    enum{ TransformFromCellCentered=0, TransformToCellCentered=1 };
-};
+AMREX_ENUM(ShiftType,
+           TransformFromCellCentered,
+           TransformToCellCentered);
 
 /**
  * \brief Class that represents the spectral space.
@@ -69,7 +70,7 @@ class SpectralKSpace
 
         SpectralShiftFactor getSpectralShiftFactor(
             const amrex::DistributionMapping& dm, int i_dim,
-            int shift_type ) const;
+            ShiftType shift_type ) const;
 
     protected:
         amrex::Array<KVectorComponent, AMREX_SPACEDIM> k_vec;

Source/FieldSolver/SpectralSolver/SpectralKSpace.cpp

@@ -145,7 +145,7 @@ SpectralKSpace::getKComponent( const DistributionMapping& dm,
 SpectralShiftFactor
 SpectralKSpace::getSpectralShiftFactor( const DistributionMapping& dm,
                                         const int i_dim,
-                                        const int shift_type ) const
+                                        const ShiftType shift_type ) const
 {
     // Initialize an empty DeviceVector in each box
     SpectralShiftFactor shift_factor( spectralspace_ba, dm );

Source/FieldSolver/SpectralSolver/SpectralKSpace_fwd.H

@@ -8,7 +8,7 @@
 #ifndef WARPX_SPECTRALKSPACE_FWD_H
 #define WARPX_SPECTRALKSPACE_FWD_H
 
-struct ShiftType;
+enum class ShiftType;
 
 class SpectralKSpace;
 

Source/Initialization/DivCleaner/ProjectionDivCleaner.cpp

@@ -104,7 +104,7 @@ void
 ProjectionDivCleaner::ReadParameters ()
 {
     // Initialize tolerance based on field precision
-    if constexpr (std::is_same<Real, float>::value) {
+    if constexpr (std::is_same_v<Real, float>) {
         m_rtol = 5e-5;
         m_atol = 0.0;
     }
@@ -337,7 +337,7 @@ WarpX::ProjectionCleanDivB() {
                 && WarpX::poisson_solver_id == PoissonSolverAlgo::Multigrid)) {
         amrex::Print() << Utils::TextMsg::Info( "Starting Projection B-Field divergence cleaner.");
 
-        if constexpr (!std::is_same<Real, double>::value) {
+        if constexpr (!std::is_same_v<Real, double>) {
             ablastr::warn_manager::WMRecordWarning("Projection Div Cleaner",
                 "WarpX is running with a field precision of SINGLE."
                 "Convergence of projection based div cleaner is not optimal and may fail.",

Source/NonlinearSolvers/NewtonSolver.H

@@ -313,7 +313,7 @@ void NewtonSolver<Vec,Ops>::Solve ( Vec&         a_U,
                          " and the relative tolerance is " << m_rtol <<
                          ". Absolute norm is " << norm_abs <<
                          " and the absolute tolerance is " << m_atol;
-       if (this->m_verbose) { amrex::Print() << convergenceMsg.str() << std::endl; }
+       if (this->m_verbose) { amrex::Print() << convergenceMsg.str() << "\n"; }
        if (m_require_convergence) {
            WARPX_ABORT_WITH_MESSAGE(convergenceMsg.str());
        } else {

Source/NonlinearSolvers/PicardSolver.H

@@ -205,7 +205,7 @@ void PicardSolver<Vec,Ops>::Solve ( Vec&         a_U,
                          " and the relative tolerance is " << m_rtol <<
                          ". Absolute norm is " << norm_abs <<
                          " and the absolute tolerance is " << m_atol;
-       if (this->m_verbose) { amrex::Print() << convergenceMsg.str() << std::endl; }
+       if (this->m_verbose) { amrex::Print() << convergenceMsg.str() << "\n"; }
        if (m_require_convergence) {
            WARPX_ABORT_WITH_MESSAGE(convergenceMsg.str());
        } else {

Source/Particles/Resampling/VelocityCoincidenceThinning.H

@@ -14,6 +14,8 @@
 #include "Utils/Parser/ParserUtils.H"
 #include "Utils/ParticleUtils.H"
 
+#include <AMReX_Algorithm.H>
+
 /**
  * \brief This class implements a particle merging scheme wherein particles
  * are clustered in phase space and particles in the same cluster is merged
@@ -66,14 +68,6 @@ public:
     */
     struct HeapSort {
 
-        AMREX_GPU_HOST_DEVICE AMREX_FORCE_INLINE
-        void swap(int &a, int &b) const
-        {
-            const auto temp = b;
-            b = a;
-            a = temp;
-        }
-
         AMREX_GPU_HOST_DEVICE AMREX_FORCE_INLINE
         void operator() (int index_array[], const int bin_array[], const int start, const int n) const
         {
@@ -84,15 +78,15 @@ public:
                 // move child through heap if it is bigger than its parent
                 while (j > 0 && bin_array[index_array[j+start]] > bin_array[index_array[(j - 1)/2 + start]]) {
                     // swap child and parent until branch is properly ordered
-                    swap(index_array[j+start], index_array[(j - 1)/2 + start]);
+                    amrex::Swap(index_array[j+start], index_array[(j - 1)/2 + start]);
                     j = (j - 1) / 2;
                 }
             }
 
             for (int i = n - 1; i > 0; i--)
             {
                 // swap value of first (now the largest value) to the new end point
-                swap(index_array[start], index_array[i+start]);
+                amrex::Swap(index_array[start], index_array[i+start]);
 
                 // remake the max heap
                 int j = 0, index;
@@ -105,7 +99,7 @@ public:
                     }
                     // if parent is smaller than child, swap parent with child having higher value
                     if (index < i && bin_array[index_array[j+start]] < bin_array[index_array[index+start]]) {
-                        swap(index_array[j+start], index_array[index+start]);
+                        amrex::Swap(index_array[j+start], index_array[index+start]);
                     }
                     j = index;
                 }

Source/Python/callbacks.cpp

@@ -33,8 +33,8 @@ void ExecutePythonCallback ( const std::string& name )
         try {
             warpx_callback_py_map[name]();
         } catch (std::exception &e) {
-            std::cerr << "Python callback '" << name << "' failed!" << std::endl;
-            std::cerr << e.what() << std::endl;
+            std::cerr << "Python callback '" << name << "' failed!" << "\n";
+            std::cerr << e.what() << "\n";
             std::exit(3);  // note: NOT amrex::Abort(), to avoid hangs with MPI
 
             // future note:

Source/Python/pyWarpX.cpp

@@ -93,7 +93,7 @@ PYBIND11_MODULE(PYWARPX_MODULE_NAME, m) {
     // TODO broken numpy if not at least v1.15.0: raise warning
     // auto numpy = py::module::import("numpy");
     // auto npversion = numpy.attr("__version__");
-    // std::cout << "numpy version: " << py::str(npversion) << std::endl;
+    // std::cout << "numpy version: " << py::str(npversion) << "\n";
 
     m.def("amrex_init",
         [](const py::list args) {

Source/ablastr/fields/EffectivePotentialPoissonSolver.H

@@ -260,7 +260,7 @@ computeEffectivePotentialPhi (
         }
 
         // Run additional operations, such as calculation of the E field for embedded boundaries
-        if constexpr (!std::is_same<T_PostPhiCalculationFunctor, std::nullopt_t>::value) {
+        if constexpr (!std::is_same_v<T_PostPhiCalculationFunctor, std::nullopt_t>) {
             if (post_phi_calculation.has_value()) {
                 post_phi_calculation.value()(mlmg, lev);
             }

Source/ablastr/fields/Interpolate.H

@@ -11,12 +11,9 @@
 #include <AMReX_IntVect.H>
 #include <AMReX_MFInterp_C.H>
 
-#include <ablastr/fields/Interpolate.H>
-
 #include <array>
 #include <optional>
 
-
 namespace ablastr::fields::details {
 
     /** Local interpolation from phi_cp to phi[lev+1]

Source/ablastr/utils/msg_logger/MsgLogger.H

@@ -79,10 +79,10 @@ namespace ablastr::utils::msg_logger
         * \brief Same as static Msg deserialize(std::vector<char>::const_iterator& it)
         * but accepting an rvalue as an argument
         *
-        * @param[in] it iterator of a byte array
+        * @param[in] rit iterator of a byte array
         * @return a Msg struct
         */
-        static Msg deserialize(std::vector<char>::const_iterator&& it);
+        static Msg deserialize(std::vector<char>::const_iterator&& rit);
     };
 
     /**
@@ -115,10 +115,10 @@ namespace ablastr::utils::msg_logger
         * \brief Same as static Msg MsgWithCounter(std::vector<char>::const_iterator& it)
         * but accepting an rvalue as an argument
         *
-        * @param[in] it iterator of a byte array
+        * @param[in] rit iterator of a byte array
         * @return a MsgWithCounter struct
         */
-        static MsgWithCounter deserialize(std::vector<char>::const_iterator&& it);
+        static MsgWithCounter deserialize(std::vector<char>::const_iterator&& rit);
     };
 
     /**
@@ -154,10 +154,10 @@ namespace ablastr::utils::msg_logger
         * \brief Same as static Msg MsgWithCounterAndRanks(std::vector<char>::const_iterator& it)
         * but accepting an rvalue as an argument
         *
-        * @param[in] it iterator of a byte array
+        * @param[in] rit iterator of a byte array
         * @return a MsgWithCounterAndRanks struct
         */
-        static MsgWithCounterAndRanks deserialize(std::vector<char>::const_iterator&& it);
+        static MsgWithCounterAndRanks deserialize(std::vector<char>::const_iterator&& rit);
     };
 
     /**

Source/ablastr/utils/msg_logger/MsgLogger.cpp

@@ -147,9 +147,10 @@ Msg Msg::deserialize (std::vector<char>::const_iterator& it)
     return msg;
 }
 
-Msg Msg::deserialize (std::vector<char>::const_iterator&& it)
+Msg Msg::deserialize (std::vector<char>::const_iterator&& rit)
 {
-    return Msg::deserialize(it);
+    auto lit = std::vector<char>::const_iterator{std::move(rit)};
+    return Msg::deserialize(lit);
 }
 
 std::vector<char> MsgWithCounter::serialize() const
@@ -174,9 +175,10 @@ MsgWithCounter MsgWithCounter::deserialize (std::vector<char>::const_iterator& i
     return msg_with_counter;
 }
 
-MsgWithCounter MsgWithCounter::deserialize (std::vector<char>::const_iterator&& it)
+MsgWithCounter MsgWithCounter::deserialize (std::vector<char>::const_iterator&& rit)
 {
-    return MsgWithCounter::deserialize(it);
+    auto lit = std::vector<char>::const_iterator{std::move(rit)};
+    return MsgWithCounter::deserialize(lit);
 }
 
 std::vector<char> MsgWithCounterAndRanks::serialize() const
@@ -205,9 +207,10 @@ MsgWithCounterAndRanks::deserialize (std::vector<char>::const_iterator& it)
 }
 
 MsgWithCounterAndRanks
-MsgWithCounterAndRanks::deserialize (std::vector<char>::const_iterator&& it)
+MsgWithCounterAndRanks::deserialize (std::vector<char>::const_iterator&& rit)
 {
-    return MsgWithCounterAndRanks::deserialize(it);
+    auto lit = std::vector<char>::const_iterator{std::move(rit)};
+    return MsgWithCounterAndRanks::deserialize(lit);
 }
 
 Logger::Logger() :

Tools/Linter/runClangTidy.sh

@@ -55,13 +55,13 @@ ${CTIDY} --version
 echo
 echo "This can be overridden by setting the environment"
 echo "variables CLANG, CLANGXX, and CLANGTIDY e.g.: "
-echo "$ export CLANG=clang-16"
-echo "$ export CLANGXX=clang++-16"
-echo "$ export CTIDCLANGTIDYY=clang-tidy-16"
+echo "$ export CLANG=clang-17"
+echo "$ export CLANGXX=clang++-17"
+echo "$ export CTIDCLANGTIDYY=clang-tidy-17"
 echo "$ ./Tools/Linter/runClangTidy.sh"
 echo
 echo "******************************************************"
-echo "* Warning: clang v16 is currently used in CI tests.  *"
+echo "* Warning: clang v17 is currently used in CI tests.  *"
 echo "* It is therefore recommended to use this version.   *"
 echo "* Otherwise, a newer version may find issues not     *"
 echo "* currently covered by CI tests while older versions *"

Tools/QedTablesUtils/Source/QedTableCommons.H

@@ -12,8 +12,8 @@ bool Contains (const ContainerType& container, const ElementType& el)
 
 void AbortWithMessage(const std::string& msg)
 {
-    std::cout << "### ABORT : " << msg << std::endl;
-    std::cout << "___________________________" << std::endl;
+    std::cerr << "### ABORT : " << msg << "\n";
+    std::cerr << "___________________________\n";
     exit(1);
 }