clang-tidy: misc-misplaced-widening-cast

Adds gmx::index to be used as type for index calculations and
comparisons.

given:
int a,b;
size_t c;
static_cast<size_t>(a+b) < c

clang-tidy warns because the static_cast does two things:
- it widens the sum from 32 bit to 64 bit
- it sign casts from signed to unsigned

If the widening is actually required (because while a and b
fit into 32 bit the sum is bigger), then this gives the
wrong result. Thus the terms should be widened prior to summing.
But this gives the wrong result if either of them is negative,
unless the terms are first widened to signed 64 bit and the sum
is then sign converted.

Additionally sign converting the signed to unsigned is unsafe
because it gives the wrong result if it negative. Converting
the unsigned is safe for 64 bit up to 2^63 which is safe to assume
won't be reached.

For all cases clang-tidy complained about (widening after sum), the
sign conversion is changed from signed->unsigned to unsigned->
signed. Introduced gmx::index to have a type for this purpose.

None of the potential bug highlighted by clang-tidy are fixed by
this change. Instead it assumes that in all cases here the intention
was just the signed conversion and no widening prior to summing
is required.

Related #2010

Change-Id: I9251196bf6ed744317868ce9bfc34876e5cfd43b

diff --git a/src/gromacs/CMakeLists.txt b/src/gromacs/CMakeLists.txt
index 5a68a4eb74662c1b0c06e878506750c4be3a27b5..b4787334daff216ac2a7b1b8868e0b95cfc5b0c1 100644 (file)
--- a/src/gromacs/CMakeLists.txt
+++ b/src/gromacs/CMakeLists.txt
@@ -280,7 +280,6 @@ if (GMX_CLANG_TIDY)
    set(CLANG_TIDY_CHECKS
        "-clang-analyzer-security.insecureAPI.strcpy"
        "bugprone-*" "misc-*" "readability-*" "performance-*" "mpi-*"
-       "-misc-misplaced-widening-cast"
        "-misc-incorrect-roundings" #TODO: #2562
        "-readability-else-after-return"
        "-readability-inconsistent-declaration-parameter-name"

diff --git a/src/gromacs/analysisdata/modules/plot.cpp b/src/gromacs/analysisdata/modules/plot.cpp
index fa8a49558f8a5ebf4bc3e5ba574b45b3916d7814..0741a07f6516217e1a70bee9b200672d8beb2fac 100644 (file)
--- a/src/gromacs/analysisdata/modules/plot.cpp
+++ b/src/gromacs/analysisdata/modules/plot.cpp
@@ -337,7 +337,7 @@ AbstractPlotModule::dataStarted(AbstractAnalysisData * /* data */)
         else
         {
             time_unit_t  time_unit
-                = static_cast<time_unit_t>(impl_->settings_.timeUnit() + 1);
+                = static_cast<time_unit_t>(impl_->settings_.timeUnit() + 1); // NOLINT(misc-misplaced-widening-cast)
             xvg_format_t xvg_format
                 = (impl_->settings_.plotFormat() > 0
                    ? static_cast<xvg_format_t>(impl_->settings_.plotFormat())

diff --git a/src/gromacs/commandline/pargs.cpp b/src/gromacs/commandline/pargs.cpp
index 90574e6435f50fa7cfebc1283f68e08213d8d3ae..232f4a0d672c2a216c75b39c6e8f6b412041495f 100644 (file)
--- a/src/gromacs/commandline/pargs.cpp
+++ b/src/gromacs/commandline/pargs.cpp
@@ -574,7 +574,7 @@ gmx_bool parse_common_args(int *argc, char *argv[], unsigned long Flags,
 
         /* set program name, command line, and default values for output options */
         output_env_init(oenv, gmx::getProgramContext(),
-                        (time_unit_t)(timeUnit + 1), bView,
+                        (time_unit_t)(timeUnit + 1), bView, // NOLINT(misc-misplaced-widening-cast)
                         (xvg_format_t)(xvgFormat + 1), 0);
 
         /* Extract Time info from arguments */

diff --git a/src/gromacs/domdec/domdec_specatomcomm.cpp b/src/gromacs/domdec/domdec_specatomcomm.cpp
index a5c33fed47adca9c86888c2b67503cc8444da8d6..4dad4aa82cd157d19e82cd9e97cd9074c6cc0b54 100644 (file)
--- a/src/gromacs/domdec/domdec_specatomcomm.cpp
+++ b/src/gromacs/domdec/domdec_specatomcomm.cpp
@@ -538,11 +538,11 @@ int setup_specat_communication(gmx_domdec_t               *dd,
             ns += spac->spas[d][1].a.size();
             nr += spac->spas[d][1].nrecv;
         }
-        if (static_cast<size_t>(vbuf_fac*ns) > spac->vbuf.size())
+        if (vbuf_fac*ns > gmx::index(spac->vbuf.size()))
         {
             spac->vbuf.resize(vbuf_fac*ns);
         }
-        if (vbuf_fac == 2 && static_cast<size_t>(vbuf_fac*nr) > spac->vbuf2.size())
+        if (vbuf_fac == 2 && vbuf_fac*nr > gmx::index(spac->vbuf2.size()))
         {
             spac->vbuf2.resize(vbuf_fac*nr);
         }

diff --git a/src/gromacs/domdec/redistribute.cpp b/src/gromacs/domdec/redistribute.cpp
index 86b6cc9ef2917d49be3672a9e656d1323442bc12..ae83ec4d79a67c90cab26cdd6e88ab6f7a429bfc 100644 (file)
--- a/src/gromacs/domdec/redistribute.cpp
+++ b/src/gromacs/domdec/redistribute.cpp
@@ -614,7 +614,7 @@ void dd_redistribute_cg(FILE *fplog, gmx_int64_t step,
             std::vector<int> &cggl_flag = comm->cggl_flag[mc];
 
             /* TODO: See if we can use push_back instead */
-            if (static_cast<size_t>((ncg[mc] + 1)*DD_CGIBS) > cggl_flag.size())
+            if ((ncg[mc] + 1)*DD_CGIBS > gmx::index(cggl_flag.size()))
             {
                 cggl_flag.resize((ncg[mc] + 1)*DD_CGIBS);
             }
@@ -940,7 +940,7 @@ void dd_redistribute_cg(FILE *fplog, gmx_int64_t step,
             else
             {
                 /* Reallocate the buffers if necessary  */
-                if (static_cast<size_t>((ncg[mc] + 1)*DD_CGIBS) > comm->cggl_flag[mc].size())
+                if ((ncg[mc] + 1)*DD_CGIBS > gmx::index(comm->cggl_flag[mc].size()))
                 {
                     comm->cggl_flag[mc].resize((ncg[mc] + 1)*DD_CGIBS);
                 }

diff --git a/src/gromacs/gmxana/gmx_bar.cpp b/src/gromacs/gmxana/gmx_bar.cpp
index 1407808fbe33e87d082cdcf6d9ef966baeb79a6a..6c1d7f6ac2ee2bb3549e6e3de5b246006dbce057 100644 (file)
--- a/src/gromacs/gmxana/gmx_bar.cpp
+++ b/src/gromacs/gmxana/gmx_bar.cpp
@@ -2971,7 +2971,7 @@ static void read_edr_rawdh_block(samples_t **smp, int *ndu, t_enxblock *blk,
     }
 
     /* make room for the data */
-    if (s->ndu_alloc < (size_t)(s->ndu + blk->sub[2].nr) )
+    if (gmx::index(s->ndu_alloc) < s->ndu + blk->sub[2].nr)
     {
         s->ndu_alloc += (s->ndu_alloc < (size_t)blk->sub[2].nr) ?
             blk->sub[2].nr*2 : s->ndu_alloc;

diff --git a/src/gromacs/mdlib/nbnxn_grid.cpp b/src/gromacs/mdlib/nbnxn_grid.cpp
index f9b4d5afe0a18efb94f40bb7f428358a56d19c21..619347c0e24ea8ed43c080ededc3e0118f1c0616 100644 (file)
--- a/src/gromacs/mdlib/nbnxn_grid.cpp
+++ b/src/gromacs/mdlib/nbnxn_grid.cpp
@@ -1303,7 +1303,7 @@ static void calc_cell_indices(nbnxn_search         *nbs,
     }
 
     /* Make sure the work array for sorting is large enough */
-    if (static_cast<size_t>(ncz_max*grid->na_sc*SGSF) > nbs->work[0].sortBuffer.size())
+    if (ncz_max*grid->na_sc*SGSF > gmx::index(nbs->work[0].sortBuffer.size()))
     {
         for (nbnxn_search_work_t &work : nbs->work)
         {

diff --git a/src/gromacs/mdlib/nbnxn_search.cpp b/src/gromacs/mdlib/nbnxn_search.cpp
index e8f5e4ad470b84b849e829f865aff8d74a66c040..7851926c5855a71bd3a960ac5d561821b304668c 100644 (file)
--- a/src/gromacs/mdlib/nbnxn_search.cpp
+++ b/src/gromacs/mdlib/nbnxn_search.cpp
@@ -1729,7 +1729,7 @@ static void make_fep_list(const nbnxn_search     *nbs,
 
     ngid = nbat->nenergrp;
 
-    if (static_cast<std::size_t>(ngid*gridj->na_cj) > sizeof(gid_cj)*8)
+    if (ngid*gridj->na_cj > gmx::index(sizeof(gid_cj)*8))
     {
         gmx_fatal(FARGS, "The Verlet scheme with %dx%d kernels and free-energy only supports up to %d energy groups",
                   gridi->na_c, gridj->na_cj, (sizeof(gid_cj)*8)/gridj->na_cj);

diff --git a/src/gromacs/trajectoryanalysis/runnercommon.cpp b/src/gromacs/trajectoryanalysis/runnercommon.cpp
index 80e70bcd3254e73142dc5aa13ef7b74b188d2ab0..59ca34b8559962fa2366cf05616db46cdeb38532 100644 (file)
--- a/src/gromacs/trajectoryanalysis/runnercommon.cpp
+++ b/src/gromacs/trajectoryanalysis/runnercommon.cpp
@@ -213,7 +213,7 @@ TrajectoryAnalysisRunnerCommon::Impl::initFirstFrame()
         return;
     }
     time_unit_t time_unit
-        = static_cast<time_unit_t>(settings_.timeUnit() + 1);
+        = static_cast<time_unit_t>(settings_.timeUnit() + 1); // NOLINT(misc-misplaced-widening-cast)
     output_env_init(&oenv_, getProgramContext(), time_unit, FALSE, exvgNONE, 0);
 
     int frflags = settings_.frflags();

diff --git a/src/gromacs/utility/basedefinitions.h b/src/gromacs/utility/basedefinitions.h
index 8e60b85058fbad7fa74073e3a1b6262bfb1e992b..6e15b51c999a1eb903dabecba3aa6fe270e5d54f 100644 (file)
--- a/src/gromacs/utility/basedefinitions.h
+++ b/src/gromacs/utility/basedefinitions.h
@@ -49,6 +49,10 @@
 #include <inttypes.h>
 #endif
 
+#if defined __cplusplus
+#include <cstddef>
+#endif
+
 /*! \brief
  * Boolean type for use in \Gromacs C code.
  *
@@ -72,6 +76,17 @@ typedef int gmx_bool;
 /** Number of gmx_bool values. */
 #define BOOL_NR 2
 
+#if defined __cplusplus
+namespace gmx
+{
+/*! \brief Integer type for indexing into arrays or vectors
+ *
+ * Same as ptrdiff_t.
+ */
+using index = std::ptrdiff_t;
+}
+#endif
+
 /*! \name Fixed-width integer types
  *
  * These types and macros provide the equivalent of 32- and 64-bit integer