clang-tidy support

.clang-tidy

@@ -0,0 +1,8 @@
+WarningsAsErrors: "*"
+UseColor: true
+HeaderFilterRegex: '^(?!MPI_api.h$|PETSc_api.h$).*' # Pulled in from python packages and not classified as system headers
+ExtraArgsBefore:
+  - '-Wno-unknown-warning-option'
+Checks: >
+  -*,
+  performance*

.github/workflows/clang-tidy.yml

@@ -0,0 +1,83 @@
+name: clang-tidy
+
+on:
+  schedule:
+    # daily at 1am
+    - cron: "0 1 * * *"
+  pull_request:
+    branches:
+      - main
+  push:
+    branches:
+      - "**"
+    tags:
+      - "v*"
+  merge_group:
+    branches:
+      - main
+  workflow_dispatch:
+
+jobs:
+  clang-tidy:
+    runs-on: ubuntu-latest
+    container: "ghcr.io/fenics/test-env:current-openmpi"
+    env:
+      PETSC_ARCH: linux-gnu-real64-32
+      OMPI_ALLOW_RUN_AS_ROOT: 1
+      OMPI_ALLOW_RUN_AS_ROOT_CONFIRM: 1
+      PRTE_MCA_rmaps_default_mapping_policy: :oversubscribe
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Install dependencies
+        run: |
+          apt-get update
+          apt-get install -y clang-tidy catch2
+          pip install -r python/build-requirements.txt
+
+      - name: Load environment variables
+        run: cat .github/workflows/fenicsx-refs.env >> $GITHUB_ENV
+
+      - name: Install FEniCS Python components
+        run: |
+          pip install git+https://github.com/FEniCS/ufl.git@${{ env.ufl_ref }}
+          pip install git+https://github.com/FEniCS/basix.git@${{ env.basix_ref }}
+          pip install git+https://github.com/FEniCS/ffcx.git@${{ env.ffcx_ref }}
+
+      - name: Configure (C++ lib) 
+        run: >
+          cmake -G Ninja
+          -B build
+          -S cpp/
+          -DCMAKE_BUILD_TYPE=Developer
+          -DENABLE_CLANG_TIDY=ON
+          -DDOLFINX_ENABLE_ADIOS2=ON
+          -DDOLFINX_ENABLE_KAHIP=ON
+          -DDOLFINX_ENABLE_PETSC=ON
+          -DDOLFINX_ENABLE_SCOTCH=ON
+          -DDOLFINX_ENABLE_SLEPC=ON
+          -DDOLFINX_ENABLE_PARMETIS=OFF
+
+      - name: Build (C++ lib)
+        run: cmake --build build
+
+      - name: Install (C++ lib)
+        run: cmake --install build
+
+      - name: Configure (C++ test)
+        run: >
+          cmake -G Ninja
+          -B build-test
+          -S cpp/test/
+          -DENABLE_CLANG_TIDY=ON
+
+      - name: Build (C++ test)
+        run: cmake --build build-test
+
+      - name: Build (Python)
+        working-directory: python
+        run: >
+          pip install .
+          --no-build-isolation
+          --verbose
+          --config-settings=cmake.define.ENABLE_CLANG_TIDY=ON

cpp/CMakeLists.txt

@@ -108,6 +108,10 @@ add_feature_info(
   "Ask Python FFCx module where to find ufcx.h header using MODULE mode. Otherwise use CONFIG mode."
 )
 
+# clang-tidy
+option(ENABLE_CLANG_TIDY "Run clang-tidy while building" OFF)
+add_feature_info(ENABLE_CLANG_TIDY ENABLE_CLANG_TIDY "Run clang-tidy while building")
+
 # ------------------------------------------------------------------------------
 # Enable or disable optional packages
 

cpp/dolfinx/CMakeLists.txt

@@ -48,6 +48,11 @@ set_target_properties(
              SOVERSION ${DOLFINX_VERSION_MAJOR}.${DOLFINX_VERSION_MINOR}
 )
 
+if(ENABLE_CLANG_TIDY)
+  find_program(CLANG_TIDY NAMES clang-tidy REQUIRED)
+  set_target_properties(dolfinx PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY};--config-file=${CMAKE_CURRENT_SOURCE_DIR}/../../.clang-tidy")
+endif()
+
 # Add git revision flag to the one affected file
 set_source_files_properties(
   common/defines.cpp

cpp/dolfinx/common/IndexMap.cpp

@@ -1183,6 +1183,7 @@ graph::AdjacencyList<int> IndexMap::index_to_dest_ranks(int tag) const
       // Build list of (ghost index, ghost position) pairs for indices
       // ghosted by this rank, and sort
       std::vector<std::pair<std::int64_t, std::int32_t>> idx_to_pos;
+      idx_to_pos.reserve(2 * _ghosts.size());
       for (auto idx : _ghosts)
         idx_to_pos.push_back({idx, idx_to_pos.size()});
       std::ranges::sort(idx_to_pos);

cpp/dolfinx/common/MPI.cpp

@@ -85,7 +85,7 @@ void dolfinx::MPI::check_error(MPI_Comm comm, int code)
     std::string error_string(MPI_MAX_ERROR_STRING, ' ');
     MPI_Error_string(code, error_string.data(), &len);
     error_string.resize(len);
-    std::cerr << error_string << std::endl;
+    std::cerr << error_string << '\n';
     MPI_Abort(comm, code);
     std::abort();
   }

cpp/dolfinx/common/MPI.h

@@ -30,7 +30,7 @@
 namespace dolfinx::MPI
 {
 /// MPI communication tags
-enum class tag : int
+enum class tag : std::uint16_t
 {
   consensus_pcx = 1200,
   consensus_pex = 1201,

cpp/dolfinx/common/Scatterer.h

@@ -34,7 +34,7 @@ class Scatterer
   using allocator_type = Allocator;
 
   /// Types of MPI communication pattern used by the Scatterer.
-  enum class type
+  enum class type : std::uint8_t
   {
     neighbor, // use MPI neighborhood collectives
     p2p       // use MPI Isend/Irecv for communication

cpp/dolfinx/common/Table.cpp

@@ -31,12 +31,12 @@ using namespace dolfinx;
 
 //-----------------------------------------------------------------------------
 Table::Table(std::string title, bool right_justify)
-    : name(title), _right_justify(right_justify)
+    : name(std::move(title)), _right_justify(right_justify)
 {
   // Do nothing
 }
 //-----------------------------------------------------------------------------
-void Table::set(std::string row, std::string col,
+void Table::set(const std::string& row, const std::string& col,
                 std::variant<std::string, int, double> value)
 {
   // Add row
@@ -49,11 +49,11 @@ void Table::set(std::string row, std::string col,
 
   // Store value
   std::pair<std::string, std::string> key(row, col);
-  _values[key] = value;
+  _values[key] = std::move(value);
 }
 //-----------------------------------------------------------------------------
-std::variant<std::string, int, double> Table::get(std::string row,
-                                                  std::string col) const
+std::variant<std::string, int, double> Table::get(const std::string& row,
+                                                  const std::string& col) const
 {
   std::pair<std::string, std::string> key(row, col);
   auto it = _values.find(key);

cpp/dolfinx/common/Table.h

@@ -6,6 +6,7 @@
 
 #pragma once
 
+#include <cstdint>
 #include <map>
 #include <mpi.h>
 #include <string>
@@ -29,7 +30,7 @@ class Table
 public:
   /// Types of MPI reduction available for Table, to get the max, min or
   /// average values over an MPI_Comm
-  enum class Reduction
+  enum class Reduction : std::uint8_t
   {
     average,
     max,
@@ -58,15 +59,15 @@ class Table
   /// @param[in] row Row name
   /// @param[in] col Column name
   /// @param[in] value The value to set
-  void set(std::string row, std::string col,
+  void set(const std::string& row, const std::string& col,
            std::variant<std::string, int, double> value);
 
   /// Get value of table entry
   /// @param[in] row Row name
   /// @param[in] col Column name
   /// @returns Returns the entry for requested row and columns
-  std::variant<std::string, int, double> get(std::string row,
-                                             std::string col) const;
+  std::variant<std::string, int, double> get(const std::string& row,
+                                             const std::string& col) const;
 
   /// Do MPI reduction on Table
   /// @param[in] comm MPI communicator

cpp/dolfinx/common/TimeLogger.cpp

@@ -21,7 +21,7 @@ TimeLogger& TimeLogger::instance()
 
 //-----------------------------------------------------------------------------
 void TimeLogger::register_timing(
-    std::string task, std::chrono::duration<double, std::ratio<1>> time)
+    const std::string& task, std::chrono::duration<double, std::ratio<1>> time)
 {
   // Print a message
   std::string line
@@ -47,7 +47,7 @@ void TimeLogger::list_timings(MPI_Comm comm, Table::Reduction reduction) const
 
   // Print just on rank 0
   if (dolfinx::MPI::rank(comm) == 0)
-    std::cout << str << std::endl;
+    std::cout << str << '\n';
 }
 //-----------------------------------------------------------------------------
 Table TimeLogger::timing_table() const
@@ -67,7 +67,7 @@ Table TimeLogger::timing_table() const
 }
 //-----------------------------------------------------------------------------
 std::pair<int, std::chrono::duration<double, std::ratio<1>>>
-TimeLogger::timing(std::string task) const
+TimeLogger::timing(const std::string& task) const
 {
   // Find timing
   auto it = _timings.find(task);

cpp/dolfinx/common/TimeLogger.h

@@ -28,7 +28,7 @@ class TimeLogger
   static TimeLogger& instance();
 
   /// Register timing (for later summary)
-  void register_timing(std::string task,
+  void register_timing(const std::string& task,
                        std::chrono::duration<double, std::ratio<1>> wall);
 
   /// Return a summary of timings and tasks in a Table
@@ -44,7 +44,7 @@ class TimeLogger
   /// @param[in] task The task name to retrieve the timing for
   /// @return Values (count, total wall time) for given task.
   std::pair<int, std::chrono::duration<double, std::ratio<1>>>
-  timing(std::string task) const;
+  timing(const std::string& task) const;
 
   /// @brief Logged elapsed times.
   /// @return Elapsed [task id: (count, total wall time)].

cpp/dolfinx/common/Timer.h

@@ -48,7 +48,9 @@ class Timer
   /// @param[in] task Name used to registered the elapsed time in the
   /// logger. If no name is set, the elapsed time is not registered in
   /// the logger.
-  Timer(std::optional<std::string> task = std::nullopt) : _task(task) {}
+  Timer(std::optional<std::string> task = std::nullopt) : _task(std::move(task))
+  {
+  }
 
   /// If timer is still running, it is stopped. Elapsed time is
   /// registered in the logger.

cpp/dolfinx/common/timing.cpp

@@ -21,7 +21,7 @@ void dolfinx::list_timings(MPI_Comm comm, Table::Reduction reduction)
 }
 //-----------------------------------------------------------------------------
 std::pair<int, std::chrono::duration<double, std::ratio<1>>>
-dolfinx::timing(std::string task)
+dolfinx::timing(const std::string& task)
 {
   return dolfinx::common::TimeLogger::instance().timing(task);
 }

cpp/dolfinx/common/timing.h

@@ -32,7 +32,7 @@ void list_timings(MPI_Comm comm,
 /// @param[in] task Name of a task
 /// @return The (count, total wall time) for the task.
 std::pair<int, std::chrono::duration<double, std::ratio<1>>>
-timing(std::string task);
+timing(const std::string& task);
 
 /// @brief Logged elapsed times.
 /// @return Elapsed [task id: (count, total wall time)].

cpp/dolfinx/fem/CoordinateElement.cpp

@@ -18,7 +18,7 @@ using namespace dolfinx::fem;
 template <std::floating_point T>
 CoordinateElement<T>::CoordinateElement(
     std::shared_ptr<const basix::FiniteElement<T>> element)
-    : _element(element)
+    : _element(std::move(element))
 {
   int degree = _element->degree();
   mesh::CellType cell = this->cell_shape();

cpp/dolfinx/fem/DirichletBC.cpp

@@ -154,6 +154,7 @@ get_remote_dofs(MPI_Comm comm, const common::IndexMap& map, int bs_map,
   // NOTE: Should we use map here or just one vector with ghosts and
   // std::distance?
   std::vector<std::pair<std::int64_t, std::int32_t>> global_local_ghosts;
+  global_local_ghosts.reserve(ghosts.size());
   const std::int32_t local_size = range[1] - range[0];
   for (std::size_t i = 0; i < ghosts.size(); ++i)
     global_local_ghosts.emplace_back(ghosts[i], i + local_size);
@@ -194,6 +195,7 @@ std::vector<std::int32_t> fem::locate_dofs_topological(
   // entity_dim
   const int num_cell_entities = mesh::cell_num_entities(cell_type, dim);
   std::vector<std::vector<int>> entity_dofs;
+  entity_dofs.reserve(num_cell_entities);
   for (int i = 0; i < num_cell_entities; ++i)
   {
     entity_dofs.push_back(
@@ -310,6 +312,7 @@ std::array<std::vector<std::int32_t>, 2> fem::locate_dofs_topological(
   // Build vector of local dofs for each cell entity
   const int num_cell_entities = mesh::cell_num_entities(cell_type, dim);
   std::vector<std::vector<int>> entity_dofs;
+  entity_dofs.reserve(num_cell_entities);
   for (int i = 0; i < num_cell_entities; ++i)
   {
     entity_dofs.push_back(

cpp/dolfinx/fem/DofMap.h

@@ -92,7 +92,7 @@ class DofMap
                                            std::vector<std::int32_t>>
   DofMap(E&& element, std::shared_ptr<const common::IndexMap> index_map,
          int index_map_bs, U&& dofmap, int bs)
-      : index_map(index_map), _index_map_bs(index_map_bs),
+      : index_map(std::move(index_map)), _index_map_bs(index_map_bs),
         _element_dof_layout(std::forward<E>(element)),
         _dofmap(std::forward<U>(dofmap)), _bs(bs),
         _shape1(_element_dof_layout.num_dofs()

cpp/dolfinx/fem/ElementDofLayout.h

@@ -7,12 +7,13 @@
 #pragma once
 
 #include <array>
+#include <cstdint>
 #include <span>
 #include <vector>
 
 namespace dolfinx::mesh
 {
-enum class CellType;
+enum class CellType : std::int8_t;
 }
 
 namespace dolfinx::fem

cpp/dolfinx/fem/FiniteElement.cpp

@@ -32,7 +32,7 @@ _build_element_list(std::vector<BasixElementData<T>> elements)
 {
   std::vector<std::shared_ptr<const FiniteElement<T>>> _e;
   std::ranges::transform(elements, std::back_inserter(_e),
-                         [](auto data)
+                         [](const auto& data)
                          {
                            auto& [e, bs, symm] = data;
                            return std::make_shared<fem::FiniteElement<T>>(e, bs,
@@ -110,7 +110,7 @@ int _compute_block_size(std::optional<std::vector<std::size_t>> value_shape,
 template <std::floating_point T>
 FiniteElement<T>::FiniteElement(
     const basix::FiniteElement<T>& element,
-    std::optional<std::vector<std::size_t>> value_shape, bool symmetric)
+    const std::optional<std::vector<std::size_t>>& value_shape, bool symmetric)
     : _value_shape(value_shape.value_or(element.value_shape())),
       _bs(_compute_block_size(value_shape, symmetric)),
       _cell_type(mesh::cell_type_from_basix_type(element.cell_type())),
@@ -161,7 +161,7 @@ FiniteElement<T>::FiniteElement(
 //-----------------------------------------------------------------------------
 template <std::floating_point T>
 FiniteElement<T>::FiniteElement(std::vector<BasixElementData<T>> elements)
-    : FiniteElement(_build_element_list(elements))
+    : FiniteElement(_build_element_list(std::move(elements)))
 {
 }
 //-----------------------------------------------------------------------------

cpp/dolfinx/fem/FiniteElement.h

@@ -22,7 +22,7 @@
 namespace dolfinx::fem
 {
 /// DOF transformation type
-enum class doftransform
+enum class doftransform : std::uint8_t
 {
   standard = 0,          ///< Standard
   transpose = 1,         ///< Transpose
@@ -68,7 +68,7 @@ class FiniteElement
   /// @param[in] symmetric Is the element a symmetric tensor? Should
   /// only set for 2nd-order tensor blocked elements.
   FiniteElement(const basix::FiniteElement<geometry_type>& element,
-                std::optional<std::vector<std::size_t>> value_shape
+                const std::optional<std::vector<std::size_t>>& value_shape
                 = std::nullopt,
                 bool symmetric = false);