Cleanups to coding standards checks

.clang-tidy

@@ -1,6 +1,7 @@
 ---
 Checks: |
   -*,
+  clang-diagnostic-*,
   bugprone-reserved-identifier,
   modernize-*,
   -modernize-use-trailing-return-type,
@@ -14,42 +15,15 @@ Checks: |
 WarningsAsErrors: '*'
 HeaderFilterRegex: '.*'
 FormatStyle:     none
-User:            arbiter
 CheckOptions:
-  - key:             llvm-else-after-return.WarnOnConditionVariables
-    value:           'false'
   - key:             modernize-loop-convert.MinConfidence
     value:           reasonable
   - key:             modernize-replace-auto-ptr.IncludeStyle
     value:           llvm
-  - key:             cert-str34-c.DiagnoseSignedUnsignedCharComparisons
-    value:           'false'
-  - key:             google-readability-namespace-comments.ShortNamespaceLines
-    value:           '10'
-  - key:             cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField
-    value:           'false'
-  - key:             cppcoreguidelines-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
-    value:           'true'
-  - key:             cert-dcl16-c.NewSuffixes
-    value:           'L;LL;LU;LLU'
-  - key:             google-readability-braces-around-statements.ShortStatementLines
-    value:           '1'
-  - key:             modernize-pass-by-value.IncludeStyle
-    value:           llvm
-  - key:             google-readability-namespace-comments.SpacesBeforeComments
-    value:           '2'
   - key:             modernize-loop-convert.MaxCopySize
     value:           '16'
-  - key:             cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors
-    value:           'true'
   - key:             modernize-use-nullptr.NullMacros
     value:           'NULL'
-  - key:             llvm-qualified-auto.AddConstToQualified
-    value:           'false'
   - key:             modernize-loop-convert.NamingStyle
     value:           CamelCase
-  - key:             llvm-else-after-return.WarnOnUnfixable
-    value:           'false'
-  - key:             google-readability-function-size.StatementThreshold
-    value:           '800'
 ...

Makefile.am

@@ -349,7 +349,7 @@ endif
 # Define VERSION for all C++ compilations and set include paths
 AM_CPPFLAGS = -I$(top_srcdir)/src -DVERSION=\"$(VERSION)\"
 
-AM_CXXFLAGS = ${LLVM_CXXFLAGS}
+AM_CXXFLAGS = ${LLVM_CXXFLAGS} -Wall -Wsign-compare -Wunreachable-code
 
 ## Convenience libraries
 ##
@@ -480,6 +480,4 @@ msw-msi:
 	light -ext WixUIExtension gambit.wixobj
 
 clang-tidy:
-	clang-tidy ${top_srcdir}/src/core/*.cc -- --std=c++20 -I ${top_srcdir}/src -DVERSION=\"$(VERSION)\"
-	clang-tidy ${top_srcdir}/src/games/*.cc ${top_srcdir}/src/games/*/*.cc -- --std=c++20 -I ${top_srcdir}/src -DVERSION=\"$(VERSION)\"
-	clang-tidy ${top_srcdir}/src/solvers/*/*.cc -- --std=c++20 -I ${top_srcdir}/src -DVERSION=\"$(VERSION)\"
+	clang-tidy ${top_srcdir}/src/core/*.cc ${top_srcdir}/src/games/*.cc ${top_srcdir}/src/games/*/*.cc ${top_srcdir}/src/solvers/*/*.cc -- --std=c++20 -I ${top_srcdir}/src -DVERSION=\"$(VERSION)\" -Wall -Wsign-compare -Wunreachable-code -Wextra-semi -Wdouble-promotion

src/core/function.cc

@@ -78,85 +78,87 @@ void ConjugatePRMinimizer::AlphaXPlusY(double alpha, const Vector<double> &x, Ve
 // These routines are drawn from comparably-named ones in
 // multimin/directional_minimize.c in GSL.
 
-void ConjugatePRMinimizer::TakeStep(const Vector<double> &x, const Vector<double> &p, double step,
-                                    double lambda, Vector<double> &x1, Vector<double> &dx)
+void ConjugatePRMinimizer::TakeStep(const Vector<double> &p_x, const Vector<double> &p_p,
+                                    double p_step, double p_lambda, Vector<double> &p_x1,
+                                    Vector<double> &p_dx)
 {
-  dx = 0.0;
-  AlphaXPlusY(-step * lambda, p, dx);
-  x1 = x;
-  AlphaXPlusY(1.0, dx, x1);
+  p_dx = 0.0;
+  AlphaXPlusY(-p_step * p_lambda, p_p, p_dx);
+  p_x1 = p_x;
+  AlphaXPlusY(1.0, p_dx, p_x1);
 }
 
-void ConjugatePRMinimizer::IntermediatePoint(const Function &fdf, const Vector<double> &x,
-                                             const Vector<double> &p, double lambda, double pg,
-                                             double stepa, double stepc, double fa, double fc,
-                                             Vector<double> &x1, Vector<double> &dx,
-                                             Vector<double> &gradient, double &step, double &f)
+void ConjugatePRMinimizer::IntermediatePoint(const Function &p_fdf, const Vector<double> &p_x,
+                                             const Vector<double> &p_p, double p_lambda,
+                                             double p_pg, double p_stepa, double p_stepc,
+                                             double p_fa, double p_fc, Vector<double> &p_x1,
+                                             Vector<double> &p_dx, Vector<double> &p_gradient,
+                                             double &p_step, double &p_f)
 {
   double stepb, fb;
 
 trial:
-  const double u = fabs(pg * lambda * stepc);
-  if ((fc - fa) + u == 0) {
+  const double u = fabs(p_pg * p_lambda * p_stepc);
+  if ((p_fc - p_fa) + u == 0) {
     // TLT: Added this check 2002/08/31 due to floating point exceptions
     // under MSW.  Not really sure how to handle this correctly.
     throw FunctionMinimizerError();
   }
-  stepb = 0.5 * stepc * u / ((fc - fa) + u);
+  stepb = 0.5 * p_stepc * u / ((p_fc - p_fa) + u);
 
-  TakeStep(x, p, stepb, lambda, x1, dx);
+  TakeStep(p_x, p_p, stepb, p_lambda, p_x1, p_dx);
 
-  fb = fdf.Value(x1);
+  fb = p_fdf.Value(p_x1);
 
-  if (fb >= fa && stepb > 0.0) {
+  if (fb >= p_fa && stepb > 0.0) {
     /* downhill step failed, reduce step-size and try again */
-    fc = fb;
-    stepc = stepb;
+    p_fc = fb;
+    p_stepc = stepb;
     goto trial;
   }
 
-  step = stepb;
-  f = fb;
-  fdf.Gradient(x1, gradient);
+  p_step = stepb;
+  p_f = fb;
+  p_fdf.Gradient(p_x1, p_gradient);
 }
 
-void ConjugatePRMinimizer::Minimize(const Function &fdf, const Vector<double> &x,
-                                    const Vector<double> &p, double lambda, double stepa,
-                                    double stepb, double stepc, double fa, double fb, double fc,
-                                    double tol, Vector<double> &x1, Vector<double> &dx1,
-                                    Vector<double> &x2, Vector<double> &dx2,
-                                    Vector<double> &gradient, double &step, double &f,
-                                    double &gnorm)
+void ConjugatePRMinimizer::Minimize(const Function &p_fdf, const Vector<double> &p_x,
+                                    const Vector<double> &p_p, double p_lambda, double p_stepa,
+                                    double p_stepb, double p_stepc, double p_fa, double p_fb,
+                                    double p_fc, double p_tol, Vector<double> &p_x1,
+                                    Vector<double> &p_dx1, Vector<double> &p_x2,
+                                    Vector<double> &p_dx2, Vector<double> &p_gradient,
+                                    double &p_step, double &p_f, double &p_gnorm)
 {
   /* Starting at (x0, f0) move along the direction p to find a minimum
      f(x0 - lambda * p), returning the new point x1 = x0-lambda*p,
      f1=f(x1) and g1 = grad(f) at x1.  */
 
-  double u = stepb;
-  double v = stepa;
-  double w = stepc;
-  double fu = fb;
-  double fv = fa;
-  double fw = fc;
+  double u = p_stepb;
+  double v = p_stepa;
+  double w = p_stepc;
+  double fu = p_fb;
+  double fv = p_fa;
+  double fw = p_fc;
 
   double old2 = fabs(w - v);
   double old1 = fabs(v - u);
 
   double stepm, fm, pg, gnorm1;
 
-  double iter = 0;
+  double num_trials = 0;
 
-  x2 = x1;
-  dx2 = dx1;
+  p_x2 = p_x1;
+  p_dx2 = p_dx1;
 
-  f = fb;
-  step = stepb;
-  gnorm = std::sqrt(gradient.NormSquared());
+  p_f = p_fb;
+  p_step = p_stepb;
+  p_gnorm = std::sqrt(p_gradient.NormSquared());
 
 mid_trial:
-  iter++;
+  num_trials++;
 
-  if (iter > 10) {
+  if (num_trials > 10) {
     return; /* MAX ITERATIONS */
   }
 
@@ -171,24 +173,24 @@ void ConjugatePRMinimizer::Minimize(const Function &fdf, const Vector<double> &x
     du = e1 / e2;
   }
 
-  if (du > 0 && du < (stepc - stepb) && fabs(du) < 0.5 * old2) {
+  if (du > 0 && du < (p_stepc - p_stepb) && fabs(du) < 0.5 * old2) {
     stepm = u + du;
   }
-  else if (du < 0 && du > (stepa - stepb) && fabs(du) < 0.5 * old2) {
+  else if (du < 0 && du > (p_stepa - p_stepb) && fabs(du) < 0.5 * old2) {
     stepm = u + du;
   }
-  else if ((stepc - stepb) > (stepb - stepa)) {
-    stepm = 0.38 * (stepc - stepb) + stepb;
+  else if ((p_stepc - p_stepb) > (p_stepb - p_stepa)) {
+    stepm = 0.38 * (p_stepc - p_stepb) + p_stepb;
   }
   else {
-    stepm = stepb - 0.38 * (stepb - stepa);
+    stepm = p_stepb - 0.38 * (p_stepb - p_stepa);
   }
 
-  TakeStep(x, p, stepm, lambda, x1, dx1);
+  TakeStep(p_x, p_p, stepm, p_lambda, p_x1, p_dx1);
 
-  fm = fdf.Value(x1);
+  fm = p_fdf.Value(p_x1);
 
-  if (fm > fb) {
+  if (fm > p_fb) {
     if (fm < fv) {
       w = v;
       v = stepm;
@@ -200,17 +202,17 @@ void ConjugatePRMinimizer::Minimize(const Function &fdf, const Vector<double> &x
       fw = fm;
     }
 
-    if (stepm < stepb) {
-      stepa = stepm;
-      fa = fm;
+    if (stepm < p_stepb) {
+      p_stepa = stepm;
+      p_fa = fm;
     }
     else {
-      stepc = stepm;
-      fc = fm;
+      p_stepc = stepm;
+      p_fc = fm;
     }
     goto mid_trial;
   }
-  else if (fm <= fb) {
+  else if (fm <= p_fb) {
     old2 = old1;
     old1 = fabs(u - stepm);
     w = v;
@@ -220,36 +222,36 @@ void ConjugatePRMinimizer::Minimize(const Function &fdf, const Vector<double> &x
     fv = fu;
     fu = fm;
 
-    x2 = x1;
-    dx2 = dx1;
+    p_x2 = p_x1;
+    p_dx2 = p_dx1;
 
-    fdf.Gradient(x1, gradient);
-    pg = p * gradient;
-    gnorm1 = std::sqrt(gradient.NormSquared());
+    p_fdf.Gradient(p_x1, p_gradient);
+    pg = p_p * p_gradient;
+    gnorm1 = std::sqrt(p_gradient.NormSquared());
 
-    f = fm;
-    step = stepm;
-    gnorm = gnorm1;
+    p_f = fm;
+    p_step = stepm;
+    p_gnorm = gnorm1;
 
     if (gnorm1 == 0.0) {
       return;
     }
 
-    if (fabs(pg * lambda / gnorm1) < tol) {
+    if (fabs(pg * p_lambda / gnorm1) < p_tol) {
       return; /* SUCCESS */
     }
 
-    if (stepm < stepb) {
-      stepc = stepb;
-      fc = fb;
-      stepb = stepm;
-      fb = fm;
+    if (stepm < p_stepb) {
+      p_stepc = p_stepb;
+      p_fc = p_fb;
+      p_stepb = stepm;
+      p_fb = fm;
     }
     else {
-      stepa = stepb;
-      fa = fb;
-      stepb = stepm;
-      fb = fm;
+      p_stepa = p_stepb;
+      p_fa = p_fb;
+      p_stepb = stepm;
+      p_fb = fm;
     }
     goto mid_trial;
   }

src/core/function.h

@@ -91,17 +91,18 @@ class ConjugatePRMinimizer : public FunctionMinimizer {
   Vector<double> g0;
 
   void AlphaXPlusY(double alpha, const Vector<double> &x, Vector<double> &y);
-  void TakeStep(const Vector<double> &x, const Vector<double> &p, double step, double lambda,
-                Vector<double> &x1, Vector<double> &dx);
-  void IntermediatePoint(const Function &fdf, const Vector<double> &x, const Vector<double> &p,
-                         double lambda, double pg, double stepa, double stepc, double fa,
-                         double fc, Vector<double> &x1, Vector<double> &dx,
-                         Vector<double> &gradient, double &step, double &f);
-  void Minimize(const Function &fdf, const Vector<double> &x, const Vector<double> &p,
-                double lambda, double stepa, double stepb, double stepc, double fa, double fb,
-                double fc, double tol, Vector<double> &x1, Vector<double> &dx1, Vector<double> &x2,
-                Vector<double> &dx2, Vector<double> &gradient, double &step, double &f,
-                double &gnorm);
+  void TakeStep(const Vector<double> &p_x, const Vector<double> &p_p, double p_step,
+                double p_lambda, Vector<double> &p_x1, Vector<double> &p_dx);
+  void IntermediatePoint(const Function &p_fdf, const Vector<double> &p_x,
+                         const Vector<double> &p_p, double p_lambda, double p_pg, double p_stepa,
+                         double p_stepc, double p_fa, double p_fc, Vector<double> &p_x1,
+                         Vector<double> &p_dx, Vector<double> &p_gradient, double &p_step,
+                         double &p_f);
+  void Minimize(const Function &p_fdf, const Vector<double> &p_x, const Vector<double> &p_p,
+                double p_lambda, double p_stepa, double p_stepb, double p_stepc, double p_fa,
+                double p_fb, double p_fc, double p_tol, Vector<double> &p_x1,
+                Vector<double> &p_dx1, Vector<double> &p_x2, Vector<double> &p_dx2,
+                Vector<double> &p_gradient, double &p_step, double &p_f, double &p_gnorm);
 };
 
 } // end namespace Gambit

src/core/integer.cc

@@ -175,10 +175,14 @@ static IntegerRep *Inew(int newlen)
     allocsiz <<= 1; // find a power of 2
   }
   allocsiz -= MALLOC_MIN_OVERHEAD;
-  // assert((unsigned long) allocsiz < MAX_INTREP_SIZE * sizeof(short));
+
+  const unsigned int repSize = (allocsiz - sizeof(IntegerRep) + sizeof(short)) / sizeof(short);
+  if (repSize > std::numeric_limits<unsigned short>::max()) {
+    throw std::overflow_error("Integer: requested precision too large");
+  }
 
   auto *rep = reinterpret_cast<IntegerRep *>(new char[allocsiz]);
-  rep->sz = (allocsiz - sizeof(IntegerRep) + sizeof(short)) / sizeof(short);
+  rep->sz = static_cast<unsigned short>(repSize);
   return rep;
 }
 

src/core/integer.h

@@ -209,8 +209,8 @@ class Integer {
 
   // coercion & conversion
 
-  int fits_in_long() const { return Iislong(rep); }
-  int fits_in_double() const { return Iisdouble(rep); }
+  bool fits_in_long() const { return Iislong(rep); }
+  bool fits_in_double() const { return Iisdouble(rep); }
 
   long as_long() const { return Itolong(rep); }
   double as_double() const { return Itodouble(rep); }