Clang-11 fixes for PME and FFT
authorArtem Zhmurov <zhmurov@gmail.com>
Fri, 23 Apr 2021 09:05:24 +0000 (09:05 +0000)
committerAndrey Alekseenko <al42and@gmail.com>
Fri, 23 Apr 2021 09:05:24 +0000 (09:05 +0000)
src/gromacs/ewald/ewald_utils.h
src/gromacs/ewald/pme_gpu_program_impl.cu
src/gromacs/ewald/pme_gpu_program_impl.h
src/gromacs/ewald/pme_load_balancing.cpp
src/gromacs/ewald/tests/.clang-tidy [new file with mode: 0644]
src/gromacs/fft/fft5d.cpp
src/gromacs/fft/fft_fftw3.cpp
src/gromacs/fft/tests/.clang-tidy [new file with mode: 0644]

index 9f77967c377a927c5a7ab55a0d0ec59218a14db2..4639d1571ba911c1d0bd36c009c0f6887edf3df6 100644 (file)
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 1991-2000, University of Groningen, The Netherlands.
  * Copyright (c) 2001-2004, The GROMACS development team.
- * Copyright (c) 2013,2014,2017,2019, by the GROMACS development team, led by
+ * Copyright (c) 2013,2014,2017,2019,2021, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -122,7 +122,7 @@ public:
      * \param[in] box        The current box matrix
      * \param[out] scaledBox Scaled copy of the box matrix.
      */
-    void scaleBox(const matrix box, matrix scaledBox)
+    void scaleBox(const matrix box, matrix scaledBox) const
     {
         GMX_ASSERT(box, "invalid source box pointer");
         GMX_ASSERT(scaledBox, "invalid target box pointer");
index 6a6196fd5cbc65318b7969307e9223ce71aceb8c..b7830fa60b99f4dae6e80229225b69f1be222723 100644 (file)
@@ -179,4 +179,4 @@ PmeGpuProgramImpl::PmeGpuProgramImpl(const DeviceContext& deviceContext) :
     // clang-format on
 }
 
-PmeGpuProgramImpl::~PmeGpuProgramImpl() {}
+PmeGpuProgramImpl::~PmeGpuProgramImpl() = default;
index 4bb3ec1a5bea22ef867bd687e35baa702cdca117..8a1e8afaf110a84a6b06d96328225bd5ee35ca6a 100644 (file)
@@ -170,6 +170,7 @@ struct PmeGpuProgramImpl
     PmeGpuProgramImpl() = delete;
     //! Constructor for the given device
     explicit PmeGpuProgramImpl(const DeviceContext& deviceContext);
+    // NOLINTNEXTLINE(performance-trivially-destructible)
     ~PmeGpuProgramImpl();
     GMX_DISALLOW_COPY_AND_ASSIGN(PmeGpuProgramImpl);
 
index 2e900d3bd4fa6550535707b4e6ad46040017f9d6..f554e43827c1e4e2eb6a2de614a3bfafdd76460d 100644 (file)
@@ -44,6 +44,7 @@
  */
 #include "gmxpre.h"
 
+#include "gromacs/utility/enumerationhelpers.h"
 #include "pme_load_balancing.h"
 
 #include <cassert>
@@ -142,22 +143,28 @@ const int c_numPostSwitchTuningIntervalSkip = 1;
 const double c_startupTimeDelay = 5.0;
 
 /*! \brief Enumeration whose values describe the effect limiting the load balancing */
-enum epmelb
+enum class PmeLoadBalancingLimit : int
 {
-    epmelblimNO,
-    epmelblimBOX,
-    epmelblimDD,
-    epmelblimPMEGRID,
-    epmelblimMAXSCALING,
-    epmelblimNR
+    No,
+    Box,
+    DD,
+    PmeGrid,
+    MaxScaling,
+    Count
 };
 
-/*! \brief Descriptive strings matching ::epmelb */
-static const char* pmelblim_str[epmelblimNR] = { "no",
-                                                 "box size",
-                                                 "domain decompostion",
-                                                 "PME grid restriction",
-                                                 "maximum allowed grid scaling" };
+/*! \brief Descriptive strings for PmeLoadBalancingLimit \c enumValue */
+static const char* enumValueToString(PmeLoadBalancingLimit enumValue)
+{
+    constexpr gmx::EnumerationArray<PmeLoadBalancingLimit, const char*> pmeLoadBalancingLimitNames = {
+        "no",
+        "box size",
+        "domain decompostion",
+        "PME grid restriction",
+        "maximum allowed grid scaling"
+    };
+    return pmeLoadBalancingLimitNames[enumValue];
+}
 
 struct pme_load_balancing_t
 {
@@ -183,7 +190,7 @@ struct pme_load_balancing_t
     int                      lower_limit;        /**< don't go below this setup index */
     int                      start;    /**< start of setup index range to consider in stage>0 */
     int                      end;      /**< end   of setup index range to consider in stage>0 */
-    int                      elimited; /**< was the balancing limited, uses enum above */
+    PmeLoadBalancingLimit    elimited; /**< was the balancing limited, uses enum above */
     CutoffScheme             cutoff_scheme; /**< Verlet or group cut-offs */
 
     int stage; /**< the current stage */
@@ -294,7 +301,7 @@ void pme_loadbal_init(pme_load_balancing_t**     pme_lb_p,
     pme_lb->lower_limit = 0;
     pme_lb->start       = 0;
     pme_lb->end         = 0;
-    pme_lb->elimited    = epmelblimNO;
+    pme_lb->elimited    = PmeLoadBalancingLimit::No;
 
     pme_lb->cycles_n = 0;
     pme_lb->cycles_c = 0;
@@ -500,7 +507,7 @@ static void print_loadbal_limited(FILE* fp_err, FILE* fp_log, int64_t step, pme_
     auto buf = gmx::formatString(
             "step %4s: the %s limits the PME load balancing to a coulomb cut-off of %.3f",
             gmx::int64ToString(step).c_str(),
-            pmelblim_str[pme_lb->elimited],
+            enumValueToString(pme_lb->elimited),
             pme_lb->setup[pme_loadbal_end(pme_lb) - 1].rcut_coulomb);
     if (fp_err != nullptr)
     {
@@ -694,7 +701,7 @@ static void pme_load_balance(pme_load_balancing_t*          pme_lb,
 
                 if (!OK)
                 {
-                    pme_lb->elimited = epmelblimPMEGRID;
+                    pme_lb->elimited = PmeLoadBalancingLimit::PmeGrid;
                 }
             }
 
@@ -702,7 +709,7 @@ static void pme_load_balance(pme_load_balancing_t*          pme_lb,
                 && pme_lb->setup[pme_lb->cur + 1].spacing > c_maxSpacingScaling * pme_lb->setup[0].spacing)
             {
                 OK               = FALSE;
-                pme_lb->elimited = epmelblimMAXSCALING;
+                pme_lb->elimited = PmeLoadBalancingLimit::MaxScaling;
             }
 
             if (OK && ir.pbcType != PbcType::No)
@@ -711,7 +718,7 @@ static void pme_load_balance(pme_load_balancing_t*          pme_lb,
                       <= max_cutoff2(ir.pbcType, box));
                 if (!OK)
                 {
-                    pme_lb->elimited = epmelblimBOX;
+                    pme_lb->elimited = PmeLoadBalancingLimit::Box;
                 }
             }
 
@@ -726,7 +733,7 @@ static void pme_load_balance(pme_load_balancing_t*          pme_lb,
                     {
                         /* Failed: do not use this setup */
                         pme_lb->cur--;
-                        pme_lb->elimited = epmelblimDD;
+                        pme_lb->elimited = PmeLoadBalancingLimit::DD;
                     }
                 }
             }
@@ -818,7 +825,7 @@ static void pme_load_balance(pme_load_balancing_t*          pme_lb,
             /* Limit the range to below the current cut-off, scan from start */
             pme_lb->end      = pme_lb->cur;
             pme_lb->cur      = pme_lb->start;
-            pme_lb->elimited = epmelblimDD;
+            pme_lb->elimited = PmeLoadBalancingLimit::DD;
             print_loadbal_limited(fp_err, fp_log, step, pme_lb);
         }
     }
@@ -1140,13 +1147,13 @@ static void print_pme_loadbal_settings(pme_load_balancing_t* pme_lb,
     fprintf(fplog, "       P P   -   P M E   L O A D   B A L A N C I N G\n");
     fprintf(fplog, "\n");
     /* Here we only warn when the optimal setting is the last one */
-    if (pme_lb->elimited != epmelblimNO && pme_lb->cur == pme_loadbal_end(pme_lb) - 1)
+    if (pme_lb->elimited != PmeLoadBalancingLimit::No && pme_lb->cur == pme_loadbal_end(pme_lb) - 1)
     {
         fprintf(fplog,
                 " NOTE: The PP/PME load balancing was limited by the %s,\n",
-                pmelblim_str[pme_lb->elimited]);
+                enumValueToString(pme_lb->elimited));
         fprintf(fplog, "       you might not have reached a good load balance.\n");
-        if (pme_lb->elimited == epmelblimDD)
+        if (pme_lb->elimited == PmeLoadBalancingLimit::DD)
         {
             fprintf(fplog, "       Try different mdrun -dd settings or lower the -dds value.\n");
         }
@@ -1179,7 +1186,7 @@ static void print_pme_loadbal_settings(pme_load_balancing_t* pme_lb,
 
 void pme_loadbal_done(pme_load_balancing_t* pme_lb, FILE* fplog, const gmx::MDLogger& mdlog, gmx_bool bNonBondedOnGPU)
 {
-    if (fplog != nullptr && (pme_lb->cur > 0 || pme_lb->elimited != epmelblimNO))
+    if (fplog != nullptr && (pme_lb->cur > 0 || pme_lb->elimited != PmeLoadBalancingLimit::No))
     {
         print_pme_loadbal_settings(pme_lb, fplog, mdlog, bNonBondedOnGPU);
     }
diff --git a/src/gromacs/ewald/tests/.clang-tidy b/src/gromacs/ewald/tests/.clang-tidy
new file mode 100644 (file)
index 0000000..0adf51e
--- /dev/null
@@ -0,0 +1,91 @@
+# List of rationales for check suppressions (where known).
+# This have to precede the list because inline comments are not
+# supported by clang-tidy.
+#
+#         -cppcoreguidelines-non-private-member-variables-in-classes,
+#         -misc-non-private-member-variables-in-classes,
+# We intend a gradual transition to conform to this guideline, but it
+# is not practical to implement yet.
+#
+#         -readability-isolate-declaration,
+# Declarations like "int a, b;" are readable. Some forms are not, and
+# those might reasonably be suggested against during code review.
+#
+#         -cppcoreguidelines-avoid-c-arrays,
+# C arrays are still necessary in many places with legacy code
+#
+#         -cppcoreguidelines-avoid-magic-numbers,
+#         -readability-magic-numbers,
+# We have many legitimate use cases for magic numbers
+#
+#         -cppcoreguidelines-macro-usage,
+# We do use too many macros, and we should fix many of them, but there
+# is no reasonable way to suppress the check e.g. in src/config.h and
+# configuring the build is a major legitimate use of macros.
+#
+#         -cppcoreguidelines-narrowing-conversions,
+#         -bugprone-narrowing-conversions
+# We have many cases where int is converted to float and we don't care
+# enough about such potential loss of precision to use explicit casts
+# in large numbers of places.
+#
+#         -google-readability-avoid-underscore-in-googletest-name
+# We need to use underscores for readability for our legacy types
+# and command-line parameter names
+#
+#         -misc-no-recursion
+# We have way too many functions and methods relying on recursion
+#
+#         -cppcoreguidelines-avoid-non-const-global-variables
+# There are quite a lot of static variables in the test code that
+# can not be replaced.
+#
+#         -modernize-avoid-bind
+# Some code needs to use std::bind and can't be modernized quickly.
+Checks:  clang-diagnostic-*,-clang-analyzer-*,-clang-analyzer-security.insecureAPI.strcpy,
+         bugprone-*,misc-*,readability-*,performance-*,mpi-*,
+         -readability-inconsistent-declaration-parameter-name,
+         -readability-function-size,-readability-else-after-return,
+         modernize-use-nullptr,modernize-use-emplace,
+         modernize-make-unique,modernize-make-shared,
+         modernize-avoid-bind,
+         modernize-use-override,
+         modernize-redundant-void-arg,modernize-use-bool-literals,
+         cppcoreguidelines-*,-cppcoreguidelines-pro-*,-cppcoreguidelines-owning-memory,
+         -cppcoreguidelines-no-malloc,-cppcoreguidelines-special-member-functions,
+         -cppcoreguidelines-avoid-goto,
+         google-*,-google-build-using-namespace,-google-explicit-constructor,
+         -google-readability-function-size,-google-readability-todo,-google-runtime-int,
+         -cppcoreguidelines-non-private-member-variables-in-classes,
+         -misc-non-private-member-variables-in-classes,
+         -readability-isolate-declaration,
+         -cppcoreguidelines-avoid-c-arrays,
+         -cppcoreguidelines-avoid-magic-numbers,
+         -readability-magic-numbers,
+         -cppcoreguidelines-macro-usage,
+         -cppcoreguidelines-narrowing-conversions,
+         -bugprone-narrowing-conversions,
+         -google-readability-avoid-underscore-in-googletest-name,
+         -cppcoreguidelines-init-variables,
+         -misc-no-recursion,
+         -cppcoreguidelines-avoid-non-const-global-variables,
+         -modernize-avoid-bind
+HeaderFilterRegex: .*
+CheckOptions:
+  - key:           cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor
+    value:         1
+  - key:           modernize-make-unique.IncludeStyle
+    value:         google
+  - key:           modernize-make-shared.IncludeStyle
+    value:         google
+  - key:           readability-implicit-bool-conversion.AllowIntegerConditions
+    value:         1
+  - key:           readability-implicit-bool-conversion.AllowPointerConditions
+    value:         1
+  - key:           bugprone-dangling-handle.HandleClasses
+    value:         std::basic_string_view; nonstd::sv_lite::basic_string_view
+# Permit passing shard pointers by value for sink parameters
+  - key:           performance-unnecessary-copy-initialization.AllowedTypes
+    value:         shared_ptr
+  - key:           performance-unnecessary-value-param.AllowedTypes
+    value:         shared_ptr
index be74cbaf6dbf951854f7323d2479ba139679756f..39fbed3d42294b2c1bdb2838f6bb36ca161f8063 100644 (file)
@@ -92,6 +92,7 @@ FILE* debug = 0;
 
 /* none of the fftw3 calls, except execute(), are thread-safe, so
    we need to serialize them with this mutex. */
+// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
 static std::mutex big_fftw_mutex;
 #    define FFTW_LOCK              \
         try                        \
index 6c704af93b6ac21afc8b17701d92c6ca1ddbdccb..f525af402609cefe4452405fc353b557fa77323a 100644 (file)
@@ -57,6 +57,7 @@
 
 /* none of the fftw3 calls, except execute(), are thread-safe, so
    we need to serialize them with this mutex. */
+// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
 static std::mutex big_fftw_mutex;
 #define FFTW_LOCK              \
     try                        \
diff --git a/src/gromacs/fft/tests/.clang-tidy b/src/gromacs/fft/tests/.clang-tidy
new file mode 100644 (file)
index 0000000..0adf51e
--- /dev/null
@@ -0,0 +1,91 @@
+# List of rationales for check suppressions (where known).
+# This have to precede the list because inline comments are not
+# supported by clang-tidy.
+#
+#         -cppcoreguidelines-non-private-member-variables-in-classes,
+#         -misc-non-private-member-variables-in-classes,
+# We intend a gradual transition to conform to this guideline, but it
+# is not practical to implement yet.
+#
+#         -readability-isolate-declaration,
+# Declarations like "int a, b;" are readable. Some forms are not, and
+# those might reasonably be suggested against during code review.
+#
+#         -cppcoreguidelines-avoid-c-arrays,
+# C arrays are still necessary in many places with legacy code
+#
+#         -cppcoreguidelines-avoid-magic-numbers,
+#         -readability-magic-numbers,
+# We have many legitimate use cases for magic numbers
+#
+#         -cppcoreguidelines-macro-usage,
+# We do use too many macros, and we should fix many of them, but there
+# is no reasonable way to suppress the check e.g. in src/config.h and
+# configuring the build is a major legitimate use of macros.
+#
+#         -cppcoreguidelines-narrowing-conversions,
+#         -bugprone-narrowing-conversions
+# We have many cases where int is converted to float and we don't care
+# enough about such potential loss of precision to use explicit casts
+# in large numbers of places.
+#
+#         -google-readability-avoid-underscore-in-googletest-name
+# We need to use underscores for readability for our legacy types
+# and command-line parameter names
+#
+#         -misc-no-recursion
+# We have way too many functions and methods relying on recursion
+#
+#         -cppcoreguidelines-avoid-non-const-global-variables
+# There are quite a lot of static variables in the test code that
+# can not be replaced.
+#
+#         -modernize-avoid-bind
+# Some code needs to use std::bind and can't be modernized quickly.
+Checks:  clang-diagnostic-*,-clang-analyzer-*,-clang-analyzer-security.insecureAPI.strcpy,
+         bugprone-*,misc-*,readability-*,performance-*,mpi-*,
+         -readability-inconsistent-declaration-parameter-name,
+         -readability-function-size,-readability-else-after-return,
+         modernize-use-nullptr,modernize-use-emplace,
+         modernize-make-unique,modernize-make-shared,
+         modernize-avoid-bind,
+         modernize-use-override,
+         modernize-redundant-void-arg,modernize-use-bool-literals,
+         cppcoreguidelines-*,-cppcoreguidelines-pro-*,-cppcoreguidelines-owning-memory,
+         -cppcoreguidelines-no-malloc,-cppcoreguidelines-special-member-functions,
+         -cppcoreguidelines-avoid-goto,
+         google-*,-google-build-using-namespace,-google-explicit-constructor,
+         -google-readability-function-size,-google-readability-todo,-google-runtime-int,
+         -cppcoreguidelines-non-private-member-variables-in-classes,
+         -misc-non-private-member-variables-in-classes,
+         -readability-isolate-declaration,
+         -cppcoreguidelines-avoid-c-arrays,
+         -cppcoreguidelines-avoid-magic-numbers,
+         -readability-magic-numbers,
+         -cppcoreguidelines-macro-usage,
+         -cppcoreguidelines-narrowing-conversions,
+         -bugprone-narrowing-conversions,
+         -google-readability-avoid-underscore-in-googletest-name,
+         -cppcoreguidelines-init-variables,
+         -misc-no-recursion,
+         -cppcoreguidelines-avoid-non-const-global-variables,
+         -modernize-avoid-bind
+HeaderFilterRegex: .*
+CheckOptions:
+  - key:           cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor
+    value:         1
+  - key:           modernize-make-unique.IncludeStyle
+    value:         google
+  - key:           modernize-make-shared.IncludeStyle
+    value:         google
+  - key:           readability-implicit-bool-conversion.AllowIntegerConditions
+    value:         1
+  - key:           readability-implicit-bool-conversion.AllowPointerConditions
+    value:         1
+  - key:           bugprone-dangling-handle.HandleClasses
+    value:         std::basic_string_view; nonstd::sv_lite::basic_string_view
+# Permit passing shard pointers by value for sink parameters
+  - key:           performance-unnecessary-copy-initialization.AllowedTypes
+    value:         shared_ptr
+  - key:           performance-unnecessary-value-param.AllowedTypes
+    value:         shared_ptr