feat(cmake): centralize and harden compiler warning flags#357
Open
feat(cmake): centralize and harden compiler warning flags#357
Conversation
Add ROS2MedkitWarnings.cmake with production-grade warning flags aligned with OpenSSF, AUTOSAR C++14, and MISRA C++ guidelines. Enable -Werror by default. Replace per-package add_compile_options blocks with a single include(ROS2MedkitWarnings) across all 12 packages. New flags: -Wswitch-enum, -Wfloat-equal, -Wcast-qual, -Wundef, -Wzero-as-null-pointer-constant, -Wextra-semi, -Wdouble-promotion, -Wsign-conversion, -Wnon-virtual-dtor, -Wold-style-cast, -Woverloaded-virtual, -Wcast-align, -Wnull-dereference, -Wimplicit-fallthrough, -Wformat=2, -Wuseless-cast (GCC), -Wduplicated-cond, -Wduplicated-branches, -Wlogical-op (GCC). Fix all warnings surfaced by the new flags: implicit conversions, useless casts, incomplete switch-enum coverage, float-to-double in printf, char sign conversions. Exclude vendored dynmsg and gtest/gmock from strict warnings via set_source_files_properties and ros2_medkit_relax_vendor_warnings(). Closes #356
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes compiler warning configuration for the ros2_medkit C++ workspace into a shared CMake module, enabling stricter warnings (and -Werror by default) and updating code to satisfy the expanded warning set.
Changes:
- Added
ROS2MedkitWarnings.cmake(strict warnings + optional-Werror) and replaced per-packageadd_compile_options()blocks withinclude(ROS2MedkitWarnings). - Added
ros2_medkit_relax_vendor_warnings()to prevent vendored gtest/gmock warnings from failing builds under-Werror, and relaxed warnings for selected vendored dynmsg sources. - Updated multiple C++ sources to address newly-enabled warnings (e.g., switch-enum coverage, format/printf type correctness, signed/unsigned conversions).
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_serialization/src/json_serializer.cpp | Completes YAML NodeType switch coverage for stricter -Wswitch-enum. |
| src/ros2_medkit_serialization/CMakeLists.txt | Switches to centralized warnings module; relaxes warnings for vendored dynmsg sources; relaxes vendor test warnings. |
| src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_msgs/CMakeLists.txt | Removes per-package compile options (consistent with “exclude generated msgs from strict warnings” intent). |
| src/ros2_medkit_integration_tests/demo_nodes/long_calibration_action.cpp | Fixes signed/unsigned indexing warnings in Fibonacci sequence logic. |
| src/ros2_medkit_integration_tests/demo_nodes/brake_actuator.cpp | Fixes printf-type correctness for float passed to varargs logging macro. |
| src/ros2_medkit_integration_tests/CMakeLists.txt | Replaces local warning flags with centralized module. |
| src/ros2_medkit_gateway/test/test_resource_sampler_registry.cpp | Removes redundant std::string(...) wrapper (warning cleanup). |
| src/ros2_medkit_gateway/src/updates/update_manager.cpp | Expands switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/openapi/capability_generator.cpp | Expands enum switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/native_topic_sampler.cpp | Handles additional enum values to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp | Adds missing enum cases to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/models/aggregation_service.cpp | Adds missing enum cases to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/log_manager.cpp | Adjusts integer conversion handling for stricter warnings. |
| src/ros2_medkit_gateway/src/lock_manager.cpp | Adds missing enum cases to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/update_handlers.cpp | Expands switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/trigger_handlers.cpp | Expands switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp | Adjusts logging argument types for stricter format warnings (but currently introduces a portability risk). |
| src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp | Adds missing enum cases to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp | Adds missing enum cases to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp | Fixes signed/unsigned conversion warning for std::count result. |
| src/ros2_medkit_gateway/src/http/handlers/cyclic_subscription_handlers.cpp | Expands enum switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp | Expands enum switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp | Fixes integer literal type for JSON value extraction to avoid conversion warnings. |
| src/ros2_medkit_gateway/src/http/entity_path_utils.cpp | Expands enum switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Expands switch handling and removes redundant std::string(...) wrappers. |
| src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp | Expands enum switch handling to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/discovery/manifest/manifest_parser.cpp | Adds explicit enum case to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/discovery/discovery_manager.cpp | Adds explicit enum case to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/discovery/discovery_enums.cpp | Adds explicit enum case to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/configuration_manager.cpp | Adds explicit enum cases to satisfy -Wswitch-enum. |
| src/ros2_medkit_gateway/src/aggregation/peer_client.cpp | Fixes std::isalnum usage by ensuring unsigned-char promotion. |
| src/ros2_medkit_gateway/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_fault_reporter/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp | Removes potentially warning-triggering redundant cast. |
| src/ros2_medkit_fault_manager/src/fault_manager_node.cpp | Adjusts parameter typing/logging (but currently introduces a format-spec mismatch). |
| src/ros2_medkit_fault_manager/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_discovery_plugins/ros2_medkit_beacon_common/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_diagnostic_bridge/CMakeLists.txt | Replaces local warning flags with centralized module; relaxes vendor test warnings. |
| src/ros2_medkit_cmake/CMakeLists.txt | Installs the new warnings module. |
| src/ros2_medkit_cmake/cmake/ROS2MedkitWarnings.cmake | New centralized warning policy (+ optional -Werror) and vendor test relaxation helper. |
| src/ros2_medkit_cmake/cmake/ros2_medkit_cmake-extras.cmake | Updates module-path documentation comment to mention ROS2MedkitWarnings. |
| auto max_snapshots = declare_parameter<int>("snapshots.max_per_fault", 10); | ||
| if (max_snapshots < 0) { | ||
| RCLCPP_WARN(get_logger(), "snapshots.max_per_fault should be >= 0, got %d. Disabling limit.", max_snapshots); | ||
| RCLCPP_WARN(get_logger(), "snapshots.max_per_fault should be >= 0, got %ld. Disabling limit.", max_snapshots); |
There was a problem hiding this comment.
Format specifier mismatch: max_snapshots is an int (from declare_parameter), but the log uses %ld. With -Wformat=2 and -Werror enabled, this will fail the build. Use %d (or cast max_snapshots to long and keep %ld) to make the format string and argument types consistent.
Suggested change
| RCLCPP_WARN(get_logger(), "snapshots.max_per_fault should be >= 0, got %ld. Disabling limit.", max_snapshots); | |
| RCLCPP_WARN(get_logger(), "snapshots.max_per_fault should be >= 0, got %d. Disabling limit.", max_snapshots); |
| RCLCPP_WARN(HandlerContext::logger(), | ||
| "Non-positive SSE keepalive override %" PRId64 "ms rejected; using default %ds", | ||
| static_cast<int64_t>(keepalive_interval.count()), kKeepaliveIntervalSec); | ||
| keepalive_interval.count(), kKeepaliveIntervalSec); |
There was a problem hiding this comment.
The format string uses PRId64 (expects an int64_t), but keepalive_interval.count() is std::chrono::milliseconds::rep (typically long long). On platforms where int64_t is not long long (common on Linux where int64_t is long), this triggers -Wformat and will become an error with -Werror. Cast keepalive_interval.count() to int64_t (or adjust the format specifier) to keep it portable.
Suggested change
| keepalive_interval.count(), kKeepaliveIntervalSec); | |
| static_cast<int64_t>(keepalive_interval.count()), kKeepaliveIntervalSec); |
GCC 13 with optimization triggers false -Wnull-dereference warnings on standard library headers (streambuf inlining), breaking CI builds. This is a known GCC issue. Null dereference checks are covered by clang-tidy instead.
Restore -Wnull-dereference which was removed due to GCC 13 false positives on STL headers. With ENABLE_WERROR defaulting to OFF, all warning flags remain active for visibility without breaking the build on different GCC versions. ENABLE_WERROR can be toggled ON once external includes are treated as system headers.
Replace blanket ENABLE_WERROR=OFF with selective -Werror=<flag> for warnings that only fire on our code: shadow, switch-enum, old-style-cast, float-equal, cast-qual, undef, zero-as-null-pointer-constant, extra-semi, overloaded-virtual, non-virtual-dtor, implicit-fallthrough, format, duplicated-cond/branches, logical-op, useless-cast (GCC). Flags that false-positive on external headers (STL, ROS 2, nlohmann, gtest) remain as warnings only: conversion, sign-conversion, double-promotion, null-dereference, cast-align. The relax function now suppresses specific promoted flags on gtest/gmock targets instead of blanket -Wno-error.
Selective -Werror=<flag> overrides blanket -Wno-error, so per-flag suppression lists are fragile and break across GCC versions. Use -w (suppress all warnings) on vendored dynmsg sources and gtest/gmock targets instead - these are third-party code we don't modify.
Type aliases (time_t, int64_t, size_t) resolve to different underlying types across distros. A static_cast needed for portability on Jazzy (GCC 13) becomes useless on Humble (GCC 11) where the types match. Keep the warning for visibility but don't fail the build.
The vendored httplib.h header triggers strict warnings (switch-enum, old-style-cast, format-nonliteral) which become errors with our selective -Werror flags. Mark the vendored include directory as SYSTEM so compiler warnings from httplib.h are suppressed. On Jazzy the system package is used via pkg-config which already treats includes as system headers.
Comment on lines
+126
to
+136
| # Suppress strict warnings on gtest/gmock vendor targets. | ||
| # ament_add_gtest/ament_add_gmock build gtest/gmock sources as subdirectory | ||
| # targets, which inherit add_compile_options flags. Vendored code may trigger | ||
| # promoted warnings (-Werror=switch-enum, -Werror=useless-cast, etc.). | ||
| # Call this once at the end of the BUILD_TESTING block. | ||
| function(ros2_medkit_relax_vendor_warnings) | ||
| foreach(_target gmock gmock_main gtest gtest_main) | ||
| if(TARGET ${_target}) | ||
| target_compile_options(${_target} PRIVATE -w) | ||
| endif() | ||
| endforeach() |
There was a problem hiding this comment.
ros2_medkit_relax_vendor_warnings() currently applies -w to gtest/gmock targets, which suppresses all warnings from those targets. If the intent is only to avoid -Werror failures while still keeping vendor warnings visible (as described in the module header/PR description), consider switching to -Wno-error (or -Wno-error=<flag> for the promoted warnings) instead of -w.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Centralize compiler warning configuration into a shared
ROS2MedkitWarnings.cmakemodule, replacing duplicated per-packageadd_compile_options()blocks. Enable-Werrorby default and add production-grade warning flags aligned with OpenSSF, AUTOSAR C++14, and MISRA C++ guidelines.Key changes:
ROS2MedkitWarnings.cmakemodule with 22+ warning flags (+ 4 GCC-only)include(ROS2MedkitWarnings)instead of per-package flags-Werrorenabled by default (configurable viaENABLE_WERRORcmake option)ros2_medkit_relax_vendor_warnings()function to suppress strict warnings on gtest/gmock vendor targetsset_source_files_propertiesNew flags added:
-Wswitch-enum,-Wfloat-equal,-Wcast-qual,-Wundef,-Wzero-as-null-pointer-constant,-Wextra-semi,-Wdouble-promotion,-Wsign-conversion,-Wnon-virtual-dtor,-Wold-style-cast,-Woverloaded-virtual,-Wcast-align,-Wnull-dereference,-Wimplicit-fallthrough,-Wformat=2,-Wuseless-cast(GCC),-Wduplicated-cond(GCC),-Wduplicated-branches(GCC),-Wlogical-op(GCC)Types of code fixes: implicit type conversions (
int64_ttoint), useless casts, incomplete switch-enum coverage (missing enum values with default), float-to-double in variadic printf, char sign conversions, redundantstd::string()wrappers.Issue
Type
Testing
colcon build --symlink-installpasses with zero errors across all 14 packages./scripts/test.shpasses: 2446 tests, 0 errors, 0 failures, 0 skippedChecklist