Fix MSVC compilation errors Disable the definition of `min`/`max` macros by defining `NOMINMAX` and prevent macro substitution in the public interface. Also, quiet floating-point comparisons are defined as template functions by the MSVC STL which causes compilation errors due to ambiguities in resolving the template parameter types. Fixes #668 Fixes #716 Fixes #718 Change-Id: I5fe7832a6a3a7ad0421a2557527528c34b88e9c7
diff --git a/CMakeLists.txt b/CMakeLists.txt index c218882..4fac3ba 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt
@@ -444,7 +444,8 @@ set_ceres_threading_model("${CERES_THREADING_MODEL}") if (BUILD_BENCHMARKS) - find_package(benchmark QUIET) + # Version 1.3 was first to provide import targets + find_package(benchmark 1.3 QUIET) if (benchmark_FOUND) message("-- Found Google benchmark library. Building Ceres benchmarks.") else() @@ -612,6 +613,15 @@ $<$<OR:$<CXX_COMPILER_ID:Clang>,$<CXX_COMPILER_ID:AppleClang>,$<CXX_COMPILER_ID:GNU>>:-Wno-deprecated-declarations> $<$<CXX_COMPILER_ID:MSVC>:/wd4996>) +if (CMAKE_VERSION VERSION_LESS 3.12) + # Disable the definiton of min/max macros within the project + if (WIN32) + add_definitions (-DNOMINMAX) + endif (WIN32) +else (CMAKE_VERSION VERSION_LESS 3.12) + add_compile_definitions($<$<BOOL:${WIN32}>:NOMINMAX>) +endif (CMAKE_VERSION VERSION_LESS 3.12) + # Configure the Ceres config.h compile options header using the current # compile options and put the configured header into the Ceres build # directory. Note that the ceres/internal subdir in <build>/config where
diff --git a/include/ceres/cubic_interpolation.h b/include/ceres/cubic_interpolation.h index 134bad2..3ca6b11 100644 --- a/include/ceres/cubic_interpolation.h +++ b/include/ceres/cubic_interpolation.h
@@ -191,7 +191,7 @@ } EIGEN_STRONG_INLINE void GetValue(const int n, double* f) const { - const int idx = std::min(std::max(begin_, n), end_ - 1) - begin_; + const int idx = (std::min)((std::max)(begin_, n), end_ - 1) - begin_; if (kInterleaved) { for (int i = 0; i < kDataDimension; ++i) { f[i] = static_cast<double>(data_[kDataDimension * idx + i]); @@ -402,9 +402,9 @@ EIGEN_STRONG_INLINE void GetValue(const int r, const int c, double* f) const { const int row_idx = - std::min(std::max(row_begin_, r), row_end_ - 1) - row_begin_; + (std::min)((std::max)(row_begin_, r), row_end_ - 1) - row_begin_; const int col_idx = - std::min(std::max(col_begin_, c), col_end_ - 1) - col_begin_; + (std::min)((std::max)(col_begin_, c), col_end_ - 1) - col_begin_; const int n = (kRowMajor) ? num_cols_ * row_idx + col_idx : num_rows_ * col_idx + row_idx;
diff --git a/include/ceres/internal/householder_vector.h b/include/ceres/internal/householder_vector.h index e5fc9bf..7700208 100644 --- a/include/ceres/internal/householder_vector.h +++ b/include/ceres/internal/householder_vector.h
@@ -82,11 +82,11 @@ v->head(v->rows() - 1) /= v_pivot; } -template <typename XVectorType, typename Scalar, int N> -Eigen::Matrix<Scalar, N, 1> ApplyHouseholderVector( +template <typename XVectorType, typename Derived> +typename Derived::PlainObject ApplyHouseholderVector( const XVectorType& y, - const Eigen::Matrix<Scalar, N, 1>& v, - const Scalar& beta) { + const Eigen::MatrixBase<Derived>& v, + const typename Derived::Scalar& beta) { return (y - v * (beta * (v.transpose() * y))); }
diff --git a/include/ceres/internal/numeric_diff.h b/include/ceres/internal/numeric_diff.h index c44733f..351845c 100644 --- a/include/ceres/internal/numeric_diff.h +++ b/include/ceres/internal/numeric_diff.h
@@ -121,7 +121,7 @@ // thus ridders_relative_initial_step_size is used. if (kMethod == RIDDERS) { min_step_size = - std::max(min_step_size, options.ridders_relative_initial_step_size); + (std::max)(min_step_size, options.ridders_relative_initial_step_size); } // For each parameter in the parameter block, use finite differences to @@ -132,7 +132,7 @@ num_residuals_internal); for (int j = 0; j < parameter_block_size_internal; ++j) { - const double delta = std::max(min_step_size, step_size(j)); + const double delta = (std::max)(min_step_size, step_size(j)); if (kMethod == RIDDERS) { if (!EvaluateRiddersJacobianColumn(functor, @@ -296,7 +296,7 @@ // norm_error is supposed to decrease as the finite difference tableau // generation progresses, serving both as an estimate for differentiation // error and as a measure of differentiation numerical stability. - double norm_error = std::numeric_limits<double>::max(); + double norm_error = (std::numeric_limits<double>::max)(); // Loop over decreasing step sizes until: // 1. Error is smaller than a given value (ridders_epsilon), @@ -342,7 +342,7 @@ options.ridders_step_shrink_factor; // Compute the difference between the previous value and the current. - double candidate_error = std::max( + double candidate_error = (std::max)( (current_candidates->col(k) - current_candidates->col(k - 1)) .norm(), (current_candidates->col(k) - previous_candidates->col(k - 1))
diff --git a/include/ceres/internal/port.h b/include/ceres/internal/port.h index 2bc44f4..4275b0e 100644 --- a/include/ceres/internal/port.h +++ b/include/ceres/internal/port.h
@@ -63,4 +63,26 @@ // 202002L) #endif // !defined(CERES_HAS_CPP20) +// Prevents symbols from being substituted by the corresponding macro definition +// under the same name. For instance, min and max are defined as macros on +// Windows (unless NOMINMAX is defined) which causes compilation errors when +// defining or referencing symbols under the same name. +// +// To be robust in all cases particularly when NOMINMAX cannot be used, use this +// macro to annotate min/max declarations/definitions. Examples: +// +// int max CERES_PREVENT_MACRO_SUBSTITUTION(); +// min CERES_PREVENT_MACRO_SUBSTITUTION(a, b); +// max CERES_PREVENT_MACRO_SUBSTITUTION(a, b); +// +// NOTE: In case the symbols for which the substitution must be prevented are +// used within another macro, the substitution must be inhibited using parens as +// +// (std::numerical_limits<double>::max)() +// +// since the helper macro will not work here. Do not use this technique in +// general case, because it will prevent argument-dependent lookup (ADL). +// +#define CERES_PREVENT_MACRO_SUBSTITUTION // Yes, it's empty + #endif // CERES_PUBLIC_INTERNAL_PORT_H_
diff --git a/include/ceres/jet.h b/include/ceres/jet.h index 10ce51e..fba1e2a 100644 --- a/include/ceres/jet.h +++ b/include/ceres/jet.h
@@ -443,15 +443,9 @@ using std::fpclassify; using std::hypot; using std::isfinite; -using std::isgreater; -using std::isgreaterequal; using std::isinf; -using std::isless; -using std::islessequal; -using std::islessgreater; using std::isnan; using std::isnormal; -using std::isunordered; using std::log; using std::log10; using std::log1p; @@ -465,6 +459,47 @@ using std::tan; using std::tanh; +// MSVC (up to 1930) defines quiet comparison functions as template functions +// which causes compilation errors due to ambiguity in the template parameter +// type resolution for using declarations in the ceres namespace. Workaround the +// issue by defining specific overload and bypass MSVC standard library +// definitions. +#if defined(_MSC_VER) +inline bool isgreater(double lhs, + double rhs) noexcept(noexcept(std::isgreater(lhs, rhs))) { + return std::isgreater(lhs, rhs); +} +inline bool isless(double lhs, + double rhs) noexcept(noexcept(std::isless(lhs, rhs))) { + return std::isless(lhs, rhs); +} +inline bool islessequal(double lhs, + double rhs) noexcept(noexcept(std::islessequal(lhs, + rhs))) { + return std::islessequal(lhs, rhs); +} +inline bool isgreaterequal(double lhs, double rhs) noexcept( + noexcept(std::isgreaterequal(lhs, rhs))) { + return std::isgreaterequal(lhs, rhs); +} +inline bool islessgreater(double lhs, double rhs) noexcept( + noexcept(std::islessgreater(lhs, rhs))) { + return std::islessgreater(lhs, rhs); +} +inline bool isunordered(double lhs, + double rhs) noexcept(noexcept(std::isunordered(lhs, + rhs))) { + return std::isunordered(lhs, rhs); +} +#else +using std::isgreater; +using std::isgreaterequal; +using std::isless; +using std::islessequal; +using std::islessgreater; +using std::isunordered; +#endif + #ifdef CERES_HAS_CPP20 using std::lerp; using std::midpoint; @@ -1246,8 +1281,9 @@ static constexpr bool tinyness_before = std::numeric_limits<T>::tinyness_before; - static constexpr ceres::Jet<T, N> min() noexcept { - return ceres::Jet<T, N>(std::numeric_limits<T>::min()); + static constexpr ceres::Jet<T, N> min + CERES_PREVENT_MACRO_SUBSTITUTION() noexcept { + return ceres::Jet<T, N>((std::numeric_limits<T>::min)()); } static constexpr ceres::Jet<T, N> lowest() noexcept { return ceres::Jet<T, N>(std::numeric_limits<T>::lowest()); @@ -1271,8 +1307,9 @@ return ceres::Jet<T, N>(std::numeric_limits<T>::denorm_min()); } - static constexpr ceres::Jet<T, N> max() noexcept { - return ceres::Jet<T, N>(std::numeric_limits<T>::max()); + static constexpr ceres::Jet<T, N> max + CERES_PREVENT_MACRO_SUBSTITUTION() noexcept { + return ceres::Jet<T, N>((std::numeric_limits<T>::max)()); } }; @@ -1326,8 +1363,8 @@ }; }; - static inline Real highest() { return Real(std::numeric_limits<T>::max()); } - static inline Real lowest() { return Real(-std::numeric_limits<T>::max()); } + static inline Real highest() { return Real((std::numeric_limits<T>::max)()); } + static inline Real lowest() { return Real(-(std::numeric_limits<T>::max)()); } }; // Specifying the return type of binary operations between Jets and scalar types
diff --git a/include/ceres/manifold.h b/include/ceres/manifold.h index 14a5792..458eec0 100644 --- a/include/ceres/manifold.h +++ b/include/ceres/manifold.h
@@ -285,6 +285,9 @@ public: ProductManifold(const ProductManifold&) = delete; ProductManifold& operator=(const ProductManifold&) = delete; + // NOTE: Do not remove the trivial destructor as this will cause linker errors + // in MSVC builds. + ~ProductManifold() override; // NOTE: The constructor takes ownership of the input // manifolds. @@ -307,7 +310,7 @@ ManifoldPtr& manifold = manifolds_[i]; manifold = std::move(manifolds_array[i]); - buffer_size_ = std::max( + buffer_size_ = (std::max)( buffer_size_, manifold->TangentSize() * manifold->AmbientSize()); ambient_size_ += manifold->AmbientSize(); tangent_size_ += manifold->TangentSize();
diff --git a/include/ceres/tiny_solver.h b/include/ceres/tiny_solver.h index 16fd2a8..86a655d 100644 --- a/include/ceres/tiny_solver.h +++ b/include/ceres/tiny_solver.h
@@ -250,7 +250,7 @@ const Scalar max_diagonal = 1e32; for (int i = 0; i < lm_diagonal_.rows(); ++i) { lm_diagonal_[i] = std::sqrt( - u * std::min(std::max(jtj_(i, i), min_diagonal), max_diagonal)); + u * (std::min)((std::max)(jtj_(i, i), min_diagonal), max_diagonal)); jtj_regularized_(i, i) += lm_diagonal_[i] * lm_diagonal_[i]; } @@ -306,7 +306,7 @@ } Scalar tmp = Scalar(2 * rho - 1); - u = u * std::max(Scalar(1 / 3.), 1 - tmp * tmp * tmp); + u = u * (std::max)(Scalar(1 / 3.), Scalar(1) - tmp * tmp * tmp); v = 2; } else {
diff --git a/internal/ceres/CMakeLists.txt b/internal/ceres/CMakeLists.txt index ea0bed8..fa538fe 100644 --- a/internal/ceres/CMakeLists.txt +++ b/internal/ceres/CMakeLists.txt
@@ -287,7 +287,7 @@ # CERES_STATIC_DEFINE is generated by the GenerateExportHeader CMake module # used to autogerate export.h. The macro should not be renamed without # updating the corresponding generate_export_header invocation. - target_compile_definitions(ceres_static PRIVATE CERES_STATIC_DEFINE) + target_compile_definitions(ceres_static PUBLIC CERES_STATIC_DEFINE) else() # In a static library build, not additional access layer is necessary as all # symbols are visible. @@ -399,14 +399,6 @@ ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}) if (BUILD_TESTING AND GFLAGS) - add_library(test_util STATIC - evaluator_test_utils.cc - numeric_diff_test_utils.cc - test_util.cc) - - target_include_directories(test_util PUBLIC ${Ceres_SOURCE_DIR}/internal) - target_link_libraries (test_util PUBLIC ceres_static gflags) - add_library(gtest gmock_gtest_all.cc gmock_main.cc) if (BUILD_SHARED_LIBS) # Define gtest-specific shared library flags for compilation. @@ -417,12 +409,21 @@ target_compile_definitions(gtest PRIVATE GTEST_CREATE_SHARED_LIBRARY=1 GFLAGS_DLL_DEFINE_FLAG=GTEST_API_ - INTERFACE GTEST_LINKED_AS_SHARED_LIBRARY=1) + INTERFACE GTEST_LINKED_AS_SHARED_LIBRARY=1 + GFLAGS_DLL_DECLARE_FLAG=GTEST_API_) endif() target_include_directories(gtest PRIVATE ${Ceres_SOURCE_DIR}/internal/ceres) target_link_libraries(gtest PRIVATE Ceres::ceres gflags) + add_library(test_util STATIC + evaluator_test_utils.cc + numeric_diff_test_utils.cc + test_util.cc) + + target_include_directories(test_util PUBLIC ${Ceres_SOURCE_DIR}/internal) + target_link_libraries (test_util PUBLIC ceres_static gflags gtest) + macro (CERES_TEST NAME) add_executable(${NAME}_test ${NAME}_test.cc) # Pull in local headers from the generated test directories when ceres_test()
diff --git a/internal/ceres/autodiff_benchmarks/CMakeLists.txt b/internal/ceres/autodiff_benchmarks/CMakeLists.txt index 610ebc3..99af152 100644 --- a/internal/ceres/autodiff_benchmarks/CMakeLists.txt +++ b/internal/ceres/autodiff_benchmarks/CMakeLists.txt
@@ -9,9 +9,3 @@ add_executable(autodiff_benchmarks autodiff_benchmarks.cc) add_dependencies_to_benchmark(autodiff_benchmarks) target_compile_options(autodiff_benchmarks PRIVATE ${CERES_BENCHMARK_FLAGS}) - -# All other flags + fast-math -list(APPEND CERES_BENCHMARK_FAST_MATH_FLAGS ${CERES_BENCHMARK_FLAGS} "-ffast-math") -add_executable(autodiff_benchmarks_fast_math autodiff_benchmarks.cc) -add_dependencies_to_benchmark(autodiff_benchmarks_fast_math) -target_compile_options(autodiff_benchmarks_fast_math PRIVATE ${CERES_BENCHMARK_FAST_MATH_FLAGS})
diff --git a/internal/ceres/dense_cholesky_test.cc b/internal/ceres/dense_cholesky_test.cc index 0e84207..c5189b8 100644 --- a/internal/ceres/dense_cholesky_test.cc +++ b/internal/ceres/dense_cholesky_test.cc
@@ -97,19 +97,25 @@ } } -INSTANTIATE_TEST_SUITE_P(_, - DenseCholeskyTest, - ::testing::Values(EIGEN +namespace { + +// NOTE: preprocessor directives in a macro are not standard conforming +decltype(auto) MakeValues() { + return ::testing::Values(EIGEN #ifndef CERES_NO_LAPACK - , - LAPACK + , + LAPACK #endif #ifndef CERES_NO_CUDA - , - CUDA + , + CUDA #endif - ), - ParamInfoToString); + ); +} + +} // namespace + +INSTANTIATE_TEST_SUITE_P(_, DenseCholeskyTest, MakeValues(), ParamInfoToString); } // namespace internal } // namespace ceres
diff --git a/internal/ceres/jet_test.cc b/internal/ceres/jet_test.cc index f06c599..6a76e1d 100644 --- a/internal/ceres/jet_test.cc +++ b/internal/ceres/jet_test.cc
@@ -30,7 +30,11 @@ // The floating-point environment access and modification is only meaningful // with the following pragma. +#ifdef _MSC_VER +#pragma fenv_access(on) +#else #pragma STDC FENV_ACCESS ON +#endif #include "ceres/jet.h"
diff --git a/internal/ceres/levenberg_marquardt_strategy_test.cc b/internal/ceres/levenberg_marquardt_strategy_test.cc index 0e7ec8d..86d35a2 100644 --- a/internal/ceres/levenberg_marquardt_strategy_test.cc +++ b/internal/ceres/levenberg_marquardt_strategy_test.cc
@@ -148,7 +148,7 @@ // are versions of glog which are not in the google namespace. using namespace google; -#if defined(_MSC_VER) +#if defined(GLOG_NO_ABBREVIATED_SEVERITIES) // Use GLOG_WARNING to support MSVC if GLOG_NO_ABBREVIATED_SEVERITIES // is defined. EXPECT_CALL(log,
diff --git a/internal/ceres/manifold.cc b/internal/ceres/manifold.cc index 451286a..1f4781e 100644 --- a/internal/ceres/manifold.cc +++ b/internal/ceres/manifold.cc
@@ -312,6 +312,20 @@ return true; } +// While the trivial destructor is not necessary, MSVC (1930) complains in +// shared library builds by issuing a linker error: +// +// bundle_adjuster.obj : error LNK2019: unresolved external symbol +// "__declspec(dllimport) const ceres::ProductManifold::`vftable'" +// (__imp_??_7ProductManifold@ceres@@6B@) referenced in function "public: +// __cdecl ceres::ProductManifold::ProductManifold<class +// ceres::QuaternionManifold,class ceres::EuclideanManifold>(class +// ceres::QuaternionManifold *,class ceres::EuclideanManifold *)" +// (??$?0VQuaternionManifold@ceres@@VEuclideanManifold@1@@ProductManifold@ceres@@QEAA@PEAVQuaternionManifold@1@PEAVEuclideanManifold@1@@Z) +// +// Providing a trivial implementation here resolves the issue. +ProductManifold::~ProductManifold() = default; + int ProductManifold::AmbientSize() const { return ambient_size_; } int ProductManifold::TangentSize() const { return tangent_size_; }
diff --git a/internal/ceres/miniglog/glog/logging.h b/internal/ceres/miniglog/glog/logging.h index f03c914..5604bd4 100644 --- a/internal/ceres/miniglog/glog/logging.h +++ b/internal/ceres/miniglog/glog/logging.h
@@ -132,7 +132,7 @@ // added, all log output is also sent to each sink through the send function. // In this implementation, WaitTillSent() is called immediately after the send. // This implementation is not thread safe. -class CERES_NO_EXPORT LogSink { +class CERES_EXPORT LogSink { public: virtual ~LogSink() = default; virtual void send(LogSeverity severity, @@ -146,7 +146,7 @@ }; // Global set of log sinks. The actual object is defined in logging.cc. -extern CERES_NO_EXPORT std::set<LogSink*> log_sinks_global; +extern CERES_EXPORT std::set<LogSink*> log_sinks_global; inline void InitGoogleLogging(const char* /* argv */) { // Do nothing; this is ignored. @@ -169,7 +169,7 @@ // defined, output is directed to std::cerr. This class should not // be directly instantiated in code, rather it should be invoked through the // use of the log macros LG, LOG, or VLOG. -class CERES_NO_EXPORT MessageLogger { +class CERES_EXPORT MessageLogger { public: MessageLogger(const char* file, int line, const char* tag, int severity) : file_(file), line_(line), tag_(tag), severity_(severity) { @@ -288,7 +288,7 @@ // This class is used to explicitly ignore values in the conditional // logging macros. This avoids compiler warnings like "value computed // is not used" and "statement has no effect". -class CERES_NO_EXPORT LoggerVoidify { +class CERES_EXPORT LoggerVoidify { public: // This has to be an operator with a precedence lower than << but // higher than ?:
diff --git a/internal/ceres/rotation_test.cc b/internal/ceres/rotation_test.cc index d97e385..d6de650 100644 --- a/internal/ceres/rotation_test.cc +++ b/internal/ceres/rotation_test.cc
@@ -88,27 +88,29 @@ MATCHER_P(IsNearQuaternion, expected, "") { // Quaternions are equivalent upto a sign change. So we will compare // both signs before declaring failure. - bool near = true; + bool is_near = true; + // NOTE: near (and far) can be defined as macros on the Windows platform (for + // ancient pascal calling convention). Do not use these identifiers. for (int i = 0; i < 4; i++) { if (fabs(arg[i] - expected[i]) > kTolerance) { - near = false; + is_near = false; break; } } - if (near) { + if (is_near) { return true; } - near = true; + is_near = true; for (int i = 0; i < 4; i++) { if (fabs(arg[i] + expected[i]) > kTolerance) { - near = false; + is_near = false; break; } } - if (near) { + if (is_near) { return true; }