From f5774f0443f71a623f84f8f8fb688cf01a3360f6 Mon Sep 17 00:00:00 2001 From: Matt Morley Date: Wed, 8 Jan 2025 21:54:44 -0700 Subject: [PATCH 1/3] Enable extra warnings --- cmake/GtsamBuildTypes.cmake | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/cmake/GtsamBuildTypes.cmake b/cmake/GtsamBuildTypes.cmake index 12d4b3a39c..55ddf61bce 100644 --- a/cmake/GtsamBuildTypes.cmake +++ b/cmake/GtsamBuildTypes.cmake @@ -111,7 +111,7 @@ if(MSVC) set(CMAKE_3_15 $) set(CMAKE_3_25 $) # Common to all configurations, next for each configuration: - set(GTSAM_COMPILE_OPTIONS_PRIVATE_COMMON /W3 /GR /EHsc /MP CACHE STRING "(User editable) Private compiler flags for all configurations.") + set(GTSAM_COMPILE_OPTIONS_PRIVATE_COMMON /W3 /WX /GR /EHsc /MP CACHE STRING "(User editable) Private compiler flags for all configurations.") set(GTSAM_COMPILE_OPTIONS_PRIVATE_DEBUG $<${CMAKE_3_15}:/MDd> $<${CMAKE_3_25}:/Zi> /Ob0 /Od /RTC1 CACHE STRING "(User editable) Private compiler flags for Debug configuration.") set(GTSAM_COMPILE_OPTIONS_PRIVATE_RELWITHDEBINFO $<${CMAKE_3_15}:/MD> /O2 $<${CMAKE_3_25}:/Zi> CACHE STRING "(User editable) Private compiler flags for RelWithDebInfo configuration.") set(GTSAM_COMPILE_OPTIONS_PRIVATE_RELEASE $<${CMAKE_3_15}:/MD> /O2 CACHE STRING "(User editable) Private compiler flags for Release configuration.") @@ -131,12 +131,15 @@ else() endif() set(GTSAM_COMPILE_OPTIONS_PRIVATE_COMMON + -Werror # Enable warnings as errors -Wall # Enable common warnings - $<$:-Wreturn-local-addr -Werror=return-local-addr> # Error: return local address - $<$:-Wreturn-stack-address -Werror=return-stack-address> # Error: return local address - $<$:-Wno-weak-template-vtables> # TODO(dellaert): don't know how to resolve + -Wpedantic # Enable pedantic warnings + $<$:-Wextra -Wno-unused-parameter> # Enable extra warnings, but ignore no-unused-parameter (as we ifdef out chunks of code) + $<$:-Wreturn-local-addr> # Error: return local address + $<$:-Wreturn-stack-address> # Error: return local address + $<$:-Wno-weak-template-vtables> # TODO(dellaert): don't know how to resolve $<$:-Wno-weak-vtables> # TODO(dellaert): don't know how to resolve - -Wreturn-type -Werror=return-type # Error on missing return() + -Wreturn-type # Error on missing return() -Wformat -Werror=format-security # Error on wrong printf() arguments $<$:${flag_override_}> # Enforce the use of the override keyword # From 3ed3c3b2e839a27433161c690550ae7e3da60ae3 Mon Sep 17 00:00:00 2001 From: Matt Morley Date: Wed, 8 Jan 2025 21:54:50 -0700 Subject: [PATCH 2/3] Bump CI GCC 11->12 --- .github/workflows/build-linux.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-linux.yml b/.github/workflows/build-linux.yml index da398ad235..3c1275a55a 100644 --- a/.github/workflows/build-linux.yml +++ b/.github/workflows/build-linux.yml @@ -27,7 +27,7 @@ jobs: name: [ ubuntu-20.04-gcc-9, ubuntu-20.04-clang-9, - ubuntu-22.04-gcc-11, + ubuntu-22.04-gcc-12, ubuntu-22.04-clang-14, ] @@ -44,7 +44,7 @@ jobs: compiler: clang version: "9" - - name: ubuntu-22.04-gcc-11 + - name: ubuntu-22.04-gcc-12 os: ubuntu-22.04 compiler: gcc version: "11" From fd2f39e271a13a8755329023e36d88c920635d18 Mon Sep 17 00:00:00 2001 From: Matt Morley Date: Wed, 8 Jan 2025 22:00:09 -0700 Subject: [PATCH 3/3] Add misc explicit operators --- gtsam/base/TestableAssertions.h | 3 ++- gtsam/geometry/Line3.cpp | 1 - gtsam/geometry/Pose3.h | 5 ++--- gtsam/geometry/Rot2.h | 3 ++- gtsam/geometry/SO4.cpp | 1 - gtsam/geometry/Similarity3.cpp | 1 - gtsam/hybrid/HybridGaussianProductFactor.cpp | 2 +- gtsam/linear/VectorValues.h | 3 ++- gtsam_unstable/base/Dummy.h | 2 ++ gtsam_unstable/base/FixedVector.h | 2 ++ gtsam_unstable/slam/Mechanization_bRn2.h | 2 ++ 11 files changed, 15 insertions(+), 10 deletions(-) diff --git a/gtsam/base/TestableAssertions.h b/gtsam/base/TestableAssertions.h index b5068ad955..f0ea79a2eb 100644 --- a/gtsam/base/TestableAssertions.h +++ b/gtsam/base/TestableAssertions.h @@ -32,7 +32,8 @@ namespace gtsam { /** * Equals testing for basic types */ -inline bool assert_equal(const Key& expected, const Key& actual, double tol = 0.0) { +inline bool assert_equal(const Key& expected, const Key& actual) { + // TODO - why isn't tol used? if(expected != actual) { std::cout << "Not equal:\nexpected: " << expected << "\nactual: " << actual << std::endl; return false; diff --git a/gtsam/geometry/Line3.cpp b/gtsam/geometry/Line3.cpp index f5cf344f51..5a4b22075e 100644 --- a/gtsam/geometry/Line3.cpp +++ b/gtsam/geometry/Line3.cpp @@ -97,7 +97,6 @@ Line3 transformTo(const Pose3 &wTc, const Line3 &wL, Rot3 cRw = wRc.inverse(); Rot3 cRl = cRw * wL.R_; - Vector2 w_ab; Vector3 t = ((wL.R_).transpose() * wTc.translation()); Vector2 c_ab(wL.a_ - t[0], wL.b_ - t[1]); diff --git a/gtsam/geometry/Pose3.h b/gtsam/geometry/Pose3.h index 54bdd7f4c4..e4896ae26f 100644 --- a/gtsam/geometry/Pose3.h +++ b/gtsam/geometry/Pose3.h @@ -56,9 +56,8 @@ class GTSAM_EXPORT Pose3: public LieGroup { /** Copy constructor */ Pose3(const Pose3& pose) = default; - // : - // R_(pose.R_), t_(pose.t_) { - // } + + Pose3& operator=(const Pose3& other) = default; /** Construct from R,t */ Pose3(const Rot3& R, const Point3& t) : diff --git a/gtsam/geometry/Rot2.h b/gtsam/geometry/Rot2.h index 7a6f25c4d0..faa0e79a28 100644 --- a/gtsam/geometry/Rot2.h +++ b/gtsam/geometry/Rot2.h @@ -53,7 +53,8 @@ namespace gtsam { /** copy constructor */ Rot2(const Rot2& r) = default; - // : Rot2(r.c_, r.s_) {} + + Rot2& operator=(const Rot2& other) = default; /// Constructor from angle in radians == exponential map at identity Rot2(double theta) : c_(cos(theta)), s_(sin(theta)) {} diff --git a/gtsam/geometry/SO4.cpp b/gtsam/geometry/SO4.cpp index 1c4920af86..e2d6c2a69d 100644 --- a/gtsam/geometry/SO4.cpp +++ b/gtsam/geometry/SO4.cpp @@ -107,7 +107,6 @@ SO4 SO4::Expmap(const Vector6& xi, ChartJacobian H) { } // Build expX = exp(xi^) - Matrix4 expX; using std::cos; using std::sin; const auto X2 = X * X; diff --git a/gtsam/geometry/Similarity3.cpp b/gtsam/geometry/Similarity3.cpp index cf2360b083..e569755efe 100644 --- a/gtsam/geometry/Similarity3.cpp +++ b/gtsam/geometry/Similarity3.cpp @@ -45,7 +45,6 @@ static Point3Pairs subtractCentroids(const Point3Pairs &abPointPairs, static double calculateScale(const Point3Pairs &d_abPointPairs, const Rot3 &aRb) { double x = 0, y = 0; - Point3 da, db; for (const auto& [da, db] : d_abPointPairs) { const Vector3 da_prime = aRb * db; y += da.transpose() * da_prime; diff --git a/gtsam/hybrid/HybridGaussianProductFactor.cpp b/gtsam/hybrid/HybridGaussianProductFactor.cpp index 280059f542..9a34ea1e95 100644 --- a/gtsam/hybrid/HybridGaussianProductFactor.cpp +++ b/gtsam/hybrid/HybridGaussianProductFactor.cpp @@ -33,7 +33,7 @@ static Y add(const Y& y1, const Y& y2) { GaussianFactorGraph result = y1.first; result.push_back(y2.first); return {result, y1.second + y2.second}; -}; +} /* *******************************************************************************/ HybridGaussianProductFactor operator+(const HybridGaussianProductFactor& a, diff --git a/gtsam/linear/VectorValues.h b/gtsam/linear/VectorValues.h index 4728706c50..35a3817457 100644 --- a/gtsam/linear/VectorValues.h +++ b/gtsam/linear/VectorValues.h @@ -106,7 +106,7 @@ namespace gtsam { template explicit VectorValues(const CONTAINER& c) : values_(c.begin(), c.end()) {} - /** Implicit copy constructor to specialize the explicit constructor from any container. */ + /** Copy constructor to specialize the explicit constructor from any container. */ VectorValues(const VectorValues& c) : values_(c.values_) {} /** Create from a pair of iterators over pair. */ @@ -119,6 +119,7 @@ namespace gtsam { /// Constructor from Vector, with Scatter VectorValues(const Vector& c, const Scatter& scatter); + // We override the copy constructor; expicitly declare operator= VectorValues& operator=(const VectorValues& other) = default; /** Create a VectorValues with the same structure as \c other, but filled with zeros. */ diff --git a/gtsam_unstable/base/Dummy.h b/gtsam_unstable/base/Dummy.h index 548bce3445..f26585ac1e 100644 --- a/gtsam_unstable/base/Dummy.h +++ b/gtsam_unstable/base/Dummy.h @@ -29,6 +29,8 @@ namespace gtsam { size_t id; Dummy(); ~Dummy(); + Dummy(const Dummy& other) = default; + Dummy& operator=(const Dummy& other) = default; void print(const std::string& s="") const ; unsigned char dummyTwoVar(unsigned char a) const ; }; diff --git a/gtsam_unstable/base/FixedVector.h b/gtsam_unstable/base/FixedVector.h index b1fff90ef5..ad6e4322d1 100644 --- a/gtsam_unstable/base/FixedVector.h +++ b/gtsam_unstable/base/FixedVector.h @@ -37,6 +37,8 @@ class FixedVector : public Eigen::Matrix { /** copy constructors */ FixedVector(const FixedVector& v) : Base(v) {} + FixedVector& operator=(const FixedVector& other) = default; + /** Convert from a variable-size vector to a fixed size vector */ FixedVector(const Vector& v) : Base(v) {} diff --git a/gtsam_unstable/slam/Mechanization_bRn2.h b/gtsam_unstable/slam/Mechanization_bRn2.h index 3d91f6b0ea..b23a4c3eec 100644 --- a/gtsam_unstable/slam/Mechanization_bRn2.h +++ b/gtsam_unstable/slam/Mechanization_bRn2.h @@ -37,6 +37,8 @@ class GTSAM_UNSTABLE_EXPORT Mechanization_bRn2 { /// Copy constructor Mechanization_bRn2(const Mechanization_bRn2& other) = default; + Mechanization_bRn2& operator=(const Mechanization_bRn2& other) = default; + /// gravity in the body frame Vector3 b_g(double g_e) const { Vector3 n_g(0, 0, g_e);