Clang-tidy Readability Checks (#526)

* turn on some of read checks

* update clang tidy

* remove const

.clang-tidy

@@ -1,49 +1,23 @@
 ---
-# Create new issues to allow
-# -readability-math-missing-parentheses
-#     eg) '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations
-# -readability-avoid-const-params-in-decls :
-#     eg) std::vector<std::string> UniqueNames(const std::function<std::string(const std::vector<std::string>& variables, const std::size_t i)> f) const;
-# -readability-non-const-parameter;
-#     eg) double* d_lower_matrix (micm/src/solver/linear_solver.cu)
-# -readability-convert-member-functions-to-static
-# -readability-identifier-length
-# -readability-braces-around-statements
-# -readability-make-member-function-const
-
-# Need discussion
-# -readability-implicit-bool-conversion: We could allow this while accepeting the two conditions specified in `Checks`
-#     eg) assert((!generated_) && "JIT Function already generated") (jit/jit_function.hpp)
-# -readability-isolate-declaration
-#     eg) double d_ymax, d_scale -> multiple declarations in a single statement reduces readability (micm/src/solver/rosenbrock.cu)
-# -readability-named-parameter: false positive cases
-
-# Have to fix later
-# -readability-avoid-unconditional-preprocessor-if:
-#     eg) #if 0 (micm/jit/jit_compiler.hpp)
-
-# Disabled
-# -readability-simplify-boolean-expr: Could hurt readability
-#     eg) return static_cast<bool>(elem == end)
-# -readability-magic-numbers,-warnings-as-errors: Doesn't allow the following example
-#     eg) parameters.c_[8] = -0.3399990352819905E+02; (micm/solver/rosenbrock_solver_parameters.hpp)
+# Here is an explanation for why some of the checks are disabled:
+#
+# -readability-identifier-length:
+#  : We want to enable this but this requires large cleanup efforts.
+#
+# -readability-convert-member-functions-to-static:
+#  : This wants to put 'static' in the inlined function defined outside the class definition
+#   but 'static' must be specified inside the class definition.
 
 Checks: >
   -*,
   readability-*,
-  -readability-named-parameter,
-  -readability-identifier-length,
-  -readability-convert-member-functions-to-static,
-  -readability-braces-around-statements,
-  -readability-math-missing-parentheses,
   -readability-avoid-const-params-in-decls,
-  -readability-avoid-unconditional-preprocessor-if,
-  -readability-make-member-function-const,
-  -readability-implicit-bool-conversion,
-  -readability-simplify-boolean-expr,
   -readability-magic-numbers,-warnings-as-errors,
-  -readability-non-const-parameter,
   -readability-isolate-declaration,
+  -readability-avoid-unconditional-preprocessor-if,
+  -readability-identifier-length,
+  -readability-convert-member-functions-to-static,
+  -readability-named-parameter,
   bugprone-*,
   -bugprone-signal-handler,
   cert-*,
@@ -74,4 +48,4 @@ CheckOptions:
   - key:             readability-implicit-bool-conversion.AllowIntegerConditions
     value:           1
   - key:             readability-implicit-bool-conversion.AllowPointerConditions
-    value:           1
+    value:           1

include/micm/jit/jit_compiler.hpp

@@ -205,11 +205,6 @@ namespace micm
             for (auto &function : module)
             {
               pass_manager->run(function);
-#if 0
-              std::cout << "Generated function definition:" << std::endl;
-              function.print(llvm::errs());
-              std::cout << std::endl;
-#endif
             }
           });
 

include/micm/jit/jit_function.hpp

@@ -133,7 +133,7 @@ namespace micm
     void EndLoop(JitLoop& loop);
 
    private:
-    llvm::AllocaInst* CreateEntryBlockAlloca(llvm::Type* type, const std::string& var_name);
+    llvm::AllocaInst* CreateEntryBlockAlloca(llvm::Type* type, const std::string& var_name) const;
   };
 
   class JitFunctionBuilder
@@ -187,8 +187,9 @@ namespace micm
       arg.arg_ = arg_iter++;
       arg.arg_->setName(arg.name_);
     }
-    for (unsigned int i = 0; i < arguments_.size(); ++i)
+    for (unsigned int i = 0; i < arguments_.size(); ++i) {
       function_->addParamAttr(i, llvm::Attribute::NoAlias);
+    }
 
     // function body
 
@@ -207,7 +208,7 @@ namespace micm
 
   std::pair<llvm::orc::ResourceTrackerSP, llvm::JITTargetAddress> JitFunction::Generate()
   {
-    assert((!generated_) && "JIT Function already generated");
+    assert((!generated_) && static_cast<bool>("JIT Function already generated"));
     std::pair<llvm::orc::ResourceTrackerSP, llvm::JITTargetAddress> ret_val;
     verifyFunction(*function_);
     ret_val.first = compiler_->GetMainJITDylib().createResourceTracker();
@@ -288,7 +289,7 @@ namespace micm
     loop.index_->addIncoming(nextIter, loop.block_);
   }
 
-  inline llvm::AllocaInst* JitFunction::CreateEntryBlockAlloca(llvm::Type* type, const std::string& var_name)
+  inline llvm::AllocaInst* JitFunction::CreateEntryBlockAlloca(llvm::Type* type, const std::string& var_name) const
   {
     llvm::IRBuilder<> TmpB(&function_->getEntryBlock(), function_->getEntryBlock().begin());
     return TmpB.CreateAlloca(type, 0, var_name.c_str());

include/micm/process/branched_rate_constant.hpp

@@ -107,8 +107,9 @@ namespace micm
   {
     double pre = parameters_.X_ * std::exp(-parameters_.Y_ / temperature);
     double Atmn = A(temperature, air_number_density);
-    if (parameters_.branch_ == BranchedRateConstantParameters::Branch::Alkoxy)
+    if (parameters_.branch_ == BranchedRateConstantParameters::Branch::Alkoxy) {
       return pre * (z_ / (z_ + Atmn));
+    }
     return pre * (Atmn / (Atmn + z_));
   }
 

include/micm/process/jit_process_set.hpp

@@ -259,8 +259,9 @@ namespace micm
         // d_rate_d_ind[i_cell] *= reactant_concentration for each reactant except ind
         for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react)
         {
-          if (i_react == i_ind)
+          if (i_react == i_ind) {
             continue;
+          }
           loop = func.StartLoop("d_rate_d_ind calc", 0, L);
           llvm::Value *react_id = llvm::ConstantInt::get(*(func.context_), llvm::APInt(64, react_ids[i_react] * L));
           ptr_index[0] = func.builder_->CreateNSWAdd(loop.index_, react_id);

include/micm/process/process_set.hpp

@@ -239,14 +239,17 @@ namespace micm
       {
         double rate = cell_rate_constants[i_rxn];
 
-        for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react)
+        for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react) {
           rate *= cell_state[react_id[i_react]];
+        }
 
-        for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react)
+        for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react) {
           cell_forcing[react_id[i_react]] -= rate;
+        }
 
-        for (std::size_t i_prod = 0; i_prod < number_of_products_[i_rxn]; ++i_prod)
+        for (std::size_t i_prod = 0; i_prod < number_of_products_[i_rxn]; ++i_prod) {
           cell_forcing[prod_id[i_prod]] += yield[i_prod] * rate;
+        }
 
         react_id += number_of_reactants_[i_rxn];
         prod_id += number_of_products_[i_rxn];
@@ -281,7 +284,7 @@ namespace micm
       std::vector<double> rate(L, 0);
       for (std::size_t i_rxn = 0; i_rxn < number_of_reactants_.size(); ++i_rxn)
       {
-        auto v_rate_subrange_begin = v_rate_constants_begin + offset_rc + i_rxn * L;
+        auto v_rate_subrange_begin = v_rate_constants_begin + offset_rc + (i_rxn * L);
         rate.assign(v_rate_subrange_begin, v_rate_subrange_begin + L);
         for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react)
           for (std::size_t i_cell = 0; i_cell < L; ++i_cell)
@@ -332,14 +335,17 @@ namespace micm
 
           for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react)
           {
-            if (i_react == i_ind)
+            if (i_react == i_ind) {
               continue;
+            }
             d_rate_d_ind *= cell_state[react_id[i_react]];
           }
-          for (std::size_t i_dep = 0; i_dep < number_of_reactants_[i_rxn]; ++i_dep)
+          for (std::size_t i_dep = 0; i_dep < number_of_reactants_[i_rxn]; ++i_dep) {
             cell_jacobian[*(flat_id++)] += d_rate_d_ind;
-          for (std::size_t i_dep = 0; i_dep < number_of_products_[i_rxn]; ++i_dep)
+          }
+          for (std::size_t i_dep = 0; i_dep < number_of_products_[i_rxn]; ++i_dep) {
             cell_jacobian[*(flat_id++)] -= yield[i_dep] * d_rate_d_ind;
+          }
         }
         react_id += number_of_reactants_[i_rxn];
         yield += number_of_products_[i_rxn];
@@ -381,13 +387,14 @@ namespace micm
       {
         for (std::size_t i_ind = 0; i_ind < number_of_reactants_[i_rxn]; ++i_ind)
         {
-          auto v_rate_subrange_begin = v_rate_constants_begin + offset_rc + i_rxn * L;
+          auto v_rate_subrange_begin = v_rate_constants_begin + offset_rc + (i_rxn * L);
           d_rate_d_ind.assign(v_rate_subrange_begin, v_rate_subrange_begin + L);
           for (std::size_t i_react = 0; i_react < number_of_reactants_[i_rxn]; ++i_react)
           {
-            if (i_react == i_ind)
+            if (i_react == i_ind) {
               continue;
-            std::size_t idx_state_variables = offset_state + react_id[i_react] * L;
+            }
+            std::size_t idx_state_variables = offset_state + (react_id[i_react] * L);
             for (std::size_t i_cell = 0; i_cell < L; ++i_cell)
               d_rate_d_ind[i_cell] *= v_state_variables[idx_state_variables + i_cell];
           }

include/micm/profiler/instrumentation.hpp

@@ -148,8 +148,9 @@ namespace micm
 
     ~InstrumentationTimer()
     {
-      if (!stopped_)
+      if (!stopped_) {
         Stop();
+      }
     }
 
     void Stop()
@@ -188,10 +189,12 @@ namespace micm
       while (srcIndex < N)
       {
         size_t matchIndex = 0;
-        while (matchIndex < K - 1 && srcIndex + matchIndex < N - 1 && expr[srcIndex + matchIndex] == remove[matchIndex])
+        while (matchIndex < K - 1 && srcIndex + matchIndex < N - 1 && expr[srcIndex + matchIndex] == remove[matchIndex]) {
           matchIndex++;
-        if (matchIndex == K - 1)
+        }
+        if (matchIndex == K - 1) {
           srcIndex += matchIndex;
+        }
         result.data_[dstIndex++] = expr[srcIndex] == '"' ? '\'' : expr[srcIndex];
         srcIndex++;
       }

include/micm/solver/jit_lu_decomposition.hpp

@@ -29,8 +29,8 @@ namespace micm
 
     JitLuDecomposition(const JitLuDecomposition &) = delete;
     JitLuDecomposition &operator=(const JitLuDecomposition &) = delete;
-    JitLuDecomposition(JitLuDecomposition &&);
-    JitLuDecomposition &operator=(JitLuDecomposition &&);
+    JitLuDecomposition(JitLuDecomposition &&other);
+    JitLuDecomposition &operator=(JitLuDecomposition &&other);
 
     /// @brief Create a JITed LU decomposer for a given sparse matrix structure
     /// @param compiler JIT compiler

include/micm/util/cuda_dense_matrix.hpp

@@ -180,8 +180,9 @@ namespace micm
     ~CudaDenseMatrix() requires(std::is_same_v<T, double>)
     {
       CHECK_CUDA_ERROR(micm::cuda::FreeVector(this->param_), "cudaFree");
-      if (this->handle_ != NULL)
+      if (this->handle_ != NULL) {
         cublasDestroy(this->handle_);
+      }
       this->param_.d_data_ = nullptr;
       this->handle_ = NULL;
     }

include/micm/util/jacobian.hpp

@@ -25,9 +25,9 @@ namespace micm
     for (auto& elem : nonzero_jacobian_elements)
       builder = builder.WithElement(elem.first, elem.second);
     // Always include diagonal elements
-    for (std::size_t i = 0; i < state_size; ++i)
+    for (std::size_t i = 0; i < state_size; ++i) {
       builder = builder.WithElement(i, i);
-
+    }
     return SparseMatrixPolicy<double>(builder);
   }
 }  // namespace micm

include/micm/util/sparse_matrix.hpp

@@ -210,9 +210,7 @@ namespace micm
       auto begin = std::next(row_ids_.begin(), row_start_[row]);
       auto end = std::next(row_ids_.begin(), row_start_[row + 1]);
       auto elem = std::find(begin, end, column);
-      if (elem == end)
-        return true;
-      return false;
+      return (elem == end);
     }
 
     std::size_t NumberOfBlocks() const

include/micm/util/vector_matrix.hpp

@@ -251,25 +251,31 @@ namespace micm
       auto y_iter = data_.begin();
       auto x_iter = x.AsVector().begin();
       const std::size_t n = std::floor(x_dim_ / L) * L * y_dim_;
-      for (std::size_t i = 0; i < n; ++i)
+      for (std::size_t i = 0; i < n; ++i) {
         *(y_iter++) += alpha * (*(x_iter++));
+      }
       const std::size_t l = x_dim_ % L;
-      for (std::size_t i = 0; i < y_dim_; ++i)
-        for (std::size_t j = 0; j < l; ++j)
+      for (std::size_t i = 0; i < y_dim_; ++i) {
+        for (std::size_t j = 0; j < l; ++j) {
           y_iter[(i * L) + j] += alpha * x_iter[(i * L) + j];
+        }
+      }
     }
 
     void ForEach(const std::function<void(T &, const T &)> f, const VectorMatrix &a)
     {
       auto this_iter = data_.begin();
       auto a_iter = a.AsVector().begin();
       const std::size_t n = std::floor(x_dim_ / L) * L * y_dim_;
-      for (std::size_t i = 0; i < n; ++i)
+      for (std::size_t i = 0; i < n; ++i) {
         f(*(this_iter++), *(a_iter++));
+      }
       const std::size_t l = x_dim_ % L;
-      for (std::size_t y = 0; y < y_dim_; ++y)
-        for (std::size_t x = 0; x < l; ++x)
+      for (std::size_t y = 0; y < y_dim_; ++y) {
+        for (std::size_t x = 0; x < l; ++x) {
           f(this_iter[(y * L) + x], a_iter[(y * L) + x]);
+        }
+      }
     }
 
     void ForEach(const std::function<void(T &, const T &, const T &)> f, const VectorMatrix &a, const VectorMatrix &b)
@@ -280,14 +286,17 @@ namespace micm
       auto a_iter = a.AsVector().begin();
       auto b_iter = b.AsVector().begin();
       const std::size_t n = std::floor(x_dim_ / L) * L * y_dim_;
-      for (std::size_t i = 0; i < n; ++i)
+      for (std::size_t i = 0; i < n; ++i) {
         f(*(this_iter++), *(a_iter++), *(b_iter++));
+      }
       const std::size_t l = x_dim_ % L;
       if (l > 0)
       {
-        for (std::size_t y = 0; y < y_dim_; ++y)
-          for (std::size_t x = 0; x < l; ++x)
+        for (std::size_t y = 0; y < y_dim_; ++y) {
+          for (std::size_t x = 0; x < l; ++x) {
             f(this_iter[(y * L) + x], a_iter[(y * L) + x], b_iter[(y * L) + x]);
+          }
+        }
       }
     }
 

src/process/process_set.cu

@@ -39,12 +39,13 @@ namespace micm
         yield_offset = 0;
         for (std::size_t i_rxn = 0; i_rxn < number_of_reactions; ++i_rxn)
         {
-          double rate = d_rate_constants[i_rxn * number_of_grid_cells + tid];
-          for (std::size_t i_react = 0; i_react < d_number_of_reactants[i_rxn]; ++i_react)
-            rate *= d_state_variables[d_reactant_ids[react_id_offset + i_react] * number_of_grid_cells + tid];
+          double rate = d_rate_constants[(i_rxn * number_of_grid_cells) + tid];
+          for (std::size_t i_react = 0; i_react < d_number_of_reactants[i_rxn]; ++i_react) {
+            rate *= d_state_variables[(d_reactant_ids[react_id_offset + i_react] * number_of_grid_cells) + tid];
+          }
           for (std::size_t i_react = 0; i_react < d_number_of_reactants[i_rxn]; ++i_react)
           {
-            d_forcing[d_reactant_ids[react_id_offset + i_react] * number_of_grid_cells + tid] -= rate;
+            d_forcing[(d_reactant_ids[react_id_offset + i_react] * number_of_grid_cells) + tid] -= rate;
           }
           for (std::size_t i_prod = 0; i_prod < d_number_of_products[i_rxn]; ++i_prod)
           {
@@ -92,12 +93,12 @@ namespace micm
           // loop over reactants in a reaction
           for (size_t i_ind = 0; i_ind < d_number_of_reactants[i_rxn]; ++i_ind)
           {
-            double d_rate_d_ind = d_rate_constants[i_rxn * number_of_grid_cells + tid];
+            double d_rate_d_ind = d_rate_constants[(i_rxn * number_of_grid_cells) + tid];
             for (size_t i_react = 0; i_react < d_number_of_reactants[i_rxn]; ++i_react)
             {
               if (i_react != i_ind)
               {
-                d_rate_d_ind *= d_state_variables[d_reactant_ids[react_ids_offset + i_react] * number_of_grid_cells + tid];
+                d_rate_d_ind *= d_state_variables[(d_reactant_ids[react_ids_offset + i_react] * number_of_grid_cells) + tid];
               }
             }
             for (size_t i_dep = 0; i_dep < d_number_of_reactants[i_rxn]; ++i_dep)
@@ -189,8 +190,9 @@ namespace micm
       cudaFree(devstruct.number_of_products_);
       cudaFree(devstruct.product_ids_);
       cudaFree(devstruct.yields_);
-      if (devstruct.jacobian_flat_ids_ != nullptr)
+      if (devstruct.jacobian_flat_ids_ != nullptr) {
         cudaFree(devstruct.jacobian_flat_ids_);
+      }
     }
 
     void SubtractJacobianTermsKernelDriver(