From 74f6bb57d22971a4888c437260d4ee27127f988a Mon Sep 17 00:00:00 2001 From: Alexander Farber Date: Wed, 24 Sep 2025 12:41:18 +0200 Subject: [PATCH 1/6] Add MSVC warning management to cmake/warnings.cmake Suppress informational warnings while keeping bug-indicating ones Fix type safety warnings in xor_fast_hash.hpp and contracted_edge_container.hpp --- CHANGELOG.md | 2 + cmake/warnings.cmake | 86 ++++++++++++++++++- .../contractor/contracted_edge_container.hpp | 6 +- include/util/xor_fast_hash.hpp | 4 +- 4 files changed, 92 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 860674d7ddc..60e61f3b0ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Unreleased - Changes from 6.0.0 + - Build: + - IMPROVED: Reduce MSVC compiler warnings by suppressing informational warnings while preserving bug-indicating warnings [#7253](https://github.com/Project-OSRM/osrm-backend/issues/7253) - Misc: - CHANGED: Update fmt library to version 11.2.0 [#7238](https://github.com/Project-OSRM/osrm-backend/issues/7238) - CHANGED: Upgrade protozero from v1.7.1 to v1.8.1 [#7239](https://github.com/Project-OSRM/osrm-backend/pull/7239) diff --git a/cmake/warnings.cmake b/cmake/warnings.cmake index fa52d6279e9..ca9f1de5dd9 100644 --- a/cmake/warnings.cmake +++ b/cmake/warnings.cmake @@ -1,7 +1,7 @@ include (CheckCXXCompilerFlag) include (CheckCCompilerFlag) -# Try to add -Wflag if compiler supports it +# Try to add -Wflag if compiler supports it (GCC/Clang) macro (add_warning flag) string(REPLACE "-" "_" underscored_flag ${flag}) string(REPLACE "+" "x" underscored_flag ${underscored_flag}) @@ -22,6 +22,37 @@ macro (add_warning flag) endif() endmacro() +# MSVC warning management macros +macro (msvc_warning_level level) + if (MSVC) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W${level}") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /W${level}") + endif() +endmacro() + +# Disable specific MSVC warning by code (e.g., 4711) +macro (msvc_disable_warning code) + if (MSVC) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd${code}") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /wd${code}") + endif() +endmacro() + +# Enable specific MSVC warning by code +macro (msvc_enable_warning code) + if (MSVC) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /w1${code}") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /w1${code}") + endif() +endmacro() + +# Disable MSVC warning for specific target +macro (target_msvc_disable_warning target code) + if (MSVC) + target_compile_options(${target} PRIVATE "/wd${code}") + endif() +endmacro() + # Try to add -Wno flag if compiler supports it macro (no_warning flag) add_warning(no-${flag}) @@ -85,4 +116,57 @@ no_warning(restrict) no_warning(free-nonheap-object) if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") no_warning(stringop-overflow) +endif() + +# MSVC-specific warning configuration +if(MSVC) + # Set warning level 3 (default level with reasonable warnings) + msvc_warning_level(3) + + # Disable excessive informational warnings that don't indicate bugs + msvc_disable_warning(4711) # Function selected for automatic inline expansion + msvc_disable_warning(4710) # Function not inlined + msvc_disable_warning(4514) # Unreferenced inline function removed + msvc_disable_warning(4820) # Padding added to struct/class + + # Disable implicit special member function warnings (often unavoidable in modern C++) + msvc_disable_warning(4626) # Assignment operator implicitly deleted + msvc_disable_warning(4625) # Copy constructor implicitly deleted + msvc_disable_warning(5027) # Move assignment operator implicitly deleted + msvc_disable_warning(5026) # Move constructor implicitly deleted + msvc_disable_warning(4623) # Default constructor implicitly deleted + + # Disable other low-value warnings + msvc_disable_warning(5219) # Implicit conversion from T to bool + msvc_disable_warning(5045) # Spectre mitigation + msvc_disable_warning(5246) # Aggregate initialization + msvc_disable_warning(4686) # Template specialization + msvc_disable_warning(5266) # const qualifier discarded + msvc_disable_warning(4800) # Implicit conversion to bool + msvc_disable_warning(4868) # Left-to-right evaluation order + msvc_disable_warning(4371) # Layout change from previous compiler version + msvc_disable_warning(4127) # Conditional expression is constant + msvc_disable_warning(4355) # 'this' used in member initializer list + msvc_disable_warning(4668) # Preprocessor macro not defined + msvc_disable_warning(5039) # Exception specification issue + msvc_disable_warning(4777) # Format string mismatch + msvc_disable_warning(5264) # Variable declared but not used + + # KEEP these warnings enabled - they indicate potential bugs: + # C4365: Signed/unsigned mismatch + # C4267: Conversion with possible loss of data + # C4244: Data conversion with possible loss + # C4242: Data conversion with possible loss + # C4458: Declaration hides class member + # C4245: Signed/unsigned mismatch in conversion + # C4389: Signed/unsigned mismatch in equality + # C4457: Declaration hides function parameter + # C4146: Unary minus applied to unsigned type + # C4456: Declaration hides previous local declaration + # C4996: Deprecated function usage + # C4100: Unreferenced formal parameter + # C4101: Unreferenced local variable + # C4061: Switch statement case not handled + + message(STATUS "MSVC warning configuration applied - suppressed informational warnings, kept bug-indicating warnings") endif() \ No newline at end of file diff --git a/include/contractor/contracted_edge_container.hpp b/include/contractor/contracted_edge_container.hpp index c7fe48fb685..febae6fbba9 100644 --- a/include/contractor/contracted_edge_container.hpp +++ b/include/contractor/contracted_edge_container.hpp @@ -61,7 +61,7 @@ struct ContractedEdgeContainer void Filter(const std::vector &filter, std::size_t index) { BOOST_ASSERT(index < sizeof(MergedFlags) * CHAR_BIT); - const MergedFlags flag = 1 << index; + const MergedFlags flag = MergedFlags{1} << index; for (auto edge_index : util::irange(0, edges.size())) { @@ -81,7 +81,7 @@ struct ContractedEdgeContainer { BOOST_ASSERT(index < sizeof(MergedFlags) * CHAR_BIT); - const MergedFlags flag = 1 << index++; + const MergedFlags flag = MergedFlags{1} << index++; auto edge_iter = edges.cbegin(); auto edge_end = edges.cend(); @@ -151,7 +151,7 @@ struct ContractedEdgeContainer std::vector> filters(index); for (const auto flag_index : util::irange(0, index)) { - MergedFlags mask = 1 << flag_index; + MergedFlags mask = MergedFlags{1} << flag_index; for (const auto flag : flags) { filters[flag_index].push_back(flag & mask); diff --git a/include/util/xor_fast_hash.hpp b/include/util/xor_fast_hash.hpp index 9e74132bc02..5e243ecb2a2 100644 --- a/include/util/xor_fast_hash.hpp +++ b/include/util/xor_fast_hash.hpp @@ -44,10 +44,10 @@ template class XORFastHash { std::mt19937 generator(1); // impl. defined but deterministic default seed - std::iota(begin(table1), end(table1), 0u); + std::iota(begin(table1), end(table1), std::uint16_t{0}); std::shuffle(begin(table1), end(table1), generator); - std::iota(begin(table2), end(table2), 0u); + std::iota(begin(table2), end(table2), std::uint16_t{0}); std::shuffle(begin(table2), end(table2), generator); } From 0cf1b3509e4c2256e1153059aa77f6fb37353dce Mon Sep 17 00:00:00 2001 From: Alexander Farber Date: Wed, 24 Sep 2025 12:48:26 +0200 Subject: [PATCH 2/6] IMPROVED -> FIXED --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60e61f3b0ec..1ea413b761b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Unreleased - Changes from 6.0.0 - Build: - - IMPROVED: Reduce MSVC compiler warnings by suppressing informational warnings while preserving bug-indicating warnings [#7253](https://github.com/Project-OSRM/osrm-backend/issues/7253) + - FIXED: Reduce MSVC compiler warnings by suppressing informational warnings while preserving bug-indicating warnings [#7253](https://github.com/Project-OSRM/osrm-backend/issues/7253) - Misc: - CHANGED: Update fmt library to version 11.2.0 [#7238](https://github.com/Project-OSRM/osrm-backend/issues/7238) - CHANGED: Upgrade protozero from v1.7.1 to v1.8.1 [#7239](https://github.com/Project-OSRM/osrm-backend/pull/7239) From 4de0dd9ab46edea0c16e4aef0c40b9f173202aad Mon Sep 17 00:00:00 2001 From: Alexander Farber Date: Wed, 24 Sep 2025 13:57:37 +0200 Subject: [PATCH 3/6] Fix warning C4146 --- include/storage/shared_datatype.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/storage/shared_datatype.hpp b/include/storage/shared_datatype.hpp index d9c6670d958..7890b8b6956 100644 --- a/include/storage/shared_datatype.hpp +++ b/include/storage/shared_datatype.hpp @@ -146,7 +146,7 @@ class ContiguousDataLayout final : public BaseDataLayout inline void *align(void *&ptr) const noexcept { const auto intptr = reinterpret_cast(ptr); - const auto aligned = (intptr - 1u + BLOCK_ALIGNMENT) & -BLOCK_ALIGNMENT; + const auto aligned = (intptr - 1u + BLOCK_ALIGNMENT) & ~(BLOCK_ALIGNMENT - 1u); return ptr = reinterpret_cast(aligned); } From 0dafb32708e1582c07c63a7c5647f36b12fb35c2 Mon Sep 17 00:00:00 2001 From: Alexander Farber Date: Wed, 24 Sep 2025 15:05:16 +0200 Subject: [PATCH 4/6] Replace -1ULL by std::numeric_limits::max() --- include/partitioner/multi_level_partition.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/partitioner/multi_level_partition.hpp b/include/partitioner/multi_level_partition.hpp index 25f417fc078..b54c0a14c93 100644 --- a/include/partitioner/multi_level_partition.hpp +++ b/include/partitioner/multi_level_partition.hpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -218,7 +219,7 @@ template class MultiLevelPartitionImpl final // Check offset for shift overflow. Offsets are strictly increasing, // so we only need the check on the last mask. PartitionID next_mask = next_offset == NUM_PARTITION_BITS - ? -1ULL + ? std::numeric_limits::max() : (1ULL << next_offset) - 1ULL; // 001100 masks[lidx++] = next_mask ^ mask; From e722e41bef10db41e1a2a0004997ba7c2eb24313 Mon Sep 17 00:00:00 2001 From: Alexander Farber Date: Wed, 24 Sep 2025 16:50:49 +0200 Subject: [PATCH 5/6] Add safe casts to fix the warning C4244 --- include/engine/douglas_peucker.hpp | 3 ++- include/engine/guidance/assemble_steps.hpp | 3 ++- include/guidance/turn_bearing.hpp | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/include/engine/douglas_peucker.hpp b/include/engine/douglas_peucker.hpp index 2be14a1ab03..e242f14134d 100644 --- a/include/engine/douglas_peucker.hpp +++ b/include/engine/douglas_peucker.hpp @@ -24,7 +24,8 @@ inline std::vector generateThreshold(double min_pixel, unsigned n const double shift = (1u << zoom) * 256; const double b = shift / 2.0; const double pixel_to_deg = 180. / b; - const std::uint64_t min_deg = min_pixel * pixel_to_deg * COORDINATE_PRECISION; + // Safe conversion: geographic coordinate calculation always produces positive integers + const std::uint64_t min_deg = static_cast(min_pixel * pixel_to_deg * COORDINATE_PRECISION); thresholds[zoom] = min_deg * min_deg; } diff --git a/include/engine/guidance/assemble_steps.hpp b/include/engine/guidance/assemble_steps.hpp index 588e402b344..21dad8eeab2 100644 --- a/include/engine/guidance/assemble_steps.hpp +++ b/include/engine/guidance/assemble_steps.hpp @@ -215,8 +215,9 @@ inline std::vector assembleSteps(const datafacade::BaseDataFacade &fa { intersection.entry.push_back(entry_class.allowsEntry(idx)); } + // Safe conversion: bearing values [0,360) fit comfortably in int16_t std::int16_t bearing_in_driving_direction = - util::bearing::reverse(std::round(bearings.first)); + static_cast(util::bearing::reverse(std::round(bearings.first))); maneuver = {intersection.location, bearing_in_driving_direction, bearings.second, diff --git a/include/guidance/turn_bearing.hpp b/include/guidance/turn_bearing.hpp index e6532f3b56f..a94d50cfeef 100644 --- a/include/guidance/turn_bearing.hpp +++ b/include/guidance/turn_bearing.hpp @@ -14,7 +14,9 @@ class TurnBearing public: static constexpr double bearing_scale = 360.0 / 256.0; // discretizes a bearing into distinct units of 1.4 degrees - TurnBearing(const double value = 0) : bearing(value / bearing_scale) + TurnBearing(const double value = 0) + // Safe conversion: [0,360) / 1.40625 = [0,256) fits exactly in uint8_t + : bearing(static_cast(value / bearing_scale)) { BOOST_ASSERT_MSG(value >= 0 && value < 360.0, "Bearing value needs to be between 0 and 360 (exclusive)"); From 073ab7023840d9ffa53ac55858f6625135c5c122 Mon Sep 17 00:00:00 2001 From: Alexander Farber Date: Wed, 24 Sep 2025 20:29:12 +0200 Subject: [PATCH 6/6] Fix CI formatting error --- include/engine/douglas_peucker.hpp | 3 ++- include/partitioner/multi_level_partition.hpp | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/engine/douglas_peucker.hpp b/include/engine/douglas_peucker.hpp index e242f14134d..e535593bbb9 100644 --- a/include/engine/douglas_peucker.hpp +++ b/include/engine/douglas_peucker.hpp @@ -25,7 +25,8 @@ inline std::vector generateThreshold(double min_pixel, unsigned n const double b = shift / 2.0; const double pixel_to_deg = 180. / b; // Safe conversion: geographic coordinate calculation always produces positive integers - const std::uint64_t min_deg = static_cast(min_pixel * pixel_to_deg * COORDINATE_PRECISION); + const std::uint64_t min_deg = + static_cast(min_pixel * pixel_to_deg * COORDINATE_PRECISION); thresholds[zoom] = min_deg * min_deg; } diff --git a/include/partitioner/multi_level_partition.hpp b/include/partitioner/multi_level_partition.hpp index b54c0a14c93..60bcb19aec8 100644 --- a/include/partitioner/multi_level_partition.hpp +++ b/include/partitioner/multi_level_partition.hpp @@ -218,9 +218,10 @@ template class MultiLevelPartitionImpl final BOOST_ASSERT(next_offset <= NUM_PARTITION_BITS); // Check offset for shift overflow. Offsets are strictly increasing, // so we only need the check on the last mask. - PartitionID next_mask = next_offset == NUM_PARTITION_BITS - ? std::numeric_limits::max() - : (1ULL << next_offset) - 1ULL; + PartitionID next_mask = + next_offset == NUM_PARTITION_BITS + ? std::numeric_limits::max() + : (1ULL << next_offset) - 1ULL; // 001100 masks[lidx++] = next_mask ^ mask; });