Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens the tracker build configuration with stricter compiler warnings/errors and applies small code cleanups to reduce warning noise and improve type/API compatibility.
Changes:
- Add stricter compile flags to the
trackertarget and treat RobotVision includes as system includes. - Remove the
clearEmptyProxyVarsworkaround and apply small warning-suppression refactors ([[maybe_unused]], unnamed params). - Align RapidJSON array indexing type with RapidJSON’s
SizeType.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tracker/CMakeLists.txt | Adds strict compile flags and marks RobotVision include dirs as SYSTEM to reduce third-party warnings. |
| tracker/src/mqtt_client.cpp | Removes the proxy-env workaround helper and its documentation block. |
| tracker/src/message_handler.cpp | Marks an intentionally-unused parameter as [[maybe_unused]]. |
| tracker/src/main.cpp | Silences unused signal handler parameter warnings by omitting the name. |
| tracker/inc/scene_parser.hpp | Uses rapidjson::SizeType for indexing to match RapidJSON APIs. |
| # Apply strict compiler flags only to the tracker target | ||
| target_compile_options(${PROJECT_NAME} | ||
| PRIVATE | ||
| -Wall -Wextra -Werror -fcf-protection=full -Wconversion -Wimplicit-fallthrough | ||
| -Wno-missing-field-initializers | ||
| ) |
There was a problem hiding this comment.
I wouldn't bother about MSVC compatibility, it is out of scope.
However, I suggest to probe with check_cxx_compiler_flag only for -fcf-protection=full flag, which seems to be compiler and HW-specific, like this:
check_cxx_compiler_flag("-fcf-protection=full" COMPILER_SUPPORTS_CF_PROTECTION)
# Apply strict compiler flags only to the tracker target
target_compile_options(${PROJECT_NAME}
PRIVATE
-Wall -Wextra -Werror -Wconversion -Wimplicit-fallthrough
-Wno-missing-field-initializers
$<$<BOOL:${COMPILER_SUPPORTS_CF_PROTECTION}>:-fcf-protection=full>
)
| * Solution: Detect empty proxy vars and unset them entirely, while | ||
| * preserving real proxy URLs for production environments that need them. | ||
| */ | ||
| void clearEmptyProxyVars() { |
There was a problem hiding this comment.
This is correct change to remove it as redundant, since clearEmptyProxyEnvVars is implemented in tracker/src/proxy_utils.cpp and used in MqttClient::MqttClient.
However, I suggest to update the comment to use the name of actually used function (clearEmptyProxyEnvVars)
| # Apply strict compiler flags only to the tracker target | ||
| target_compile_options(${PROJECT_NAME} | ||
| PRIVATE | ||
| -Wall -Wextra -Werror -fcf-protection=full -Wconversion -Wimplicit-fallthrough | ||
| -Wno-missing-field-initializers | ||
| ) |
There was a problem hiding this comment.
I wouldn't bother about MSVC compatibility, it is out of scope.
However, I suggest to probe with check_cxx_compiler_flag only for -fcf-protection=full flag, which seems to be compiler and HW-specific, like this:
check_cxx_compiler_flag("-fcf-protection=full" COMPILER_SUPPORTS_CF_PROTECTION)
# Apply strict compiler flags only to the tracker target
target_compile_options(${PROJECT_NAME}
PRIVATE
-Wall -Wextra -Werror -Wconversion -Wimplicit-fallthrough
-Wno-missing-field-initializers
$<$<BOOL:${COMPILER_SUPPORTS_CF_PROTECTION}>:-fcf-protection=full>
)
- Probe -fcf-protection=full via check_cxx_compiler_flag before use - Guard RobotVision SYSTEM include with if(TARGET RobotVision) - Fix stale function reference in docker-compose comment
📝 Description
This pull request introduces several improvements focused on code quality, build configuration, and minor refactoring for clarity and maintainability. The most notable changes include enforcing stricter compiler flags, improving third-party include handling, and making minor code cleanups and type corrections.
Build configuration improvements:
-Wall,-Wextra,-Werror,-fcf-protection=full,-Wconversion,-Wimplicit-fallthrough) to the tracker target to enforce higher code quality and catch more issues at compile time. Also, treated RobotVision headers as system includes to suppress third-party warnings. (tracker/CMakeLists.txt)Code cleanup and minor refactoring:
clearEmptyProxyVarsfunction and its associated comments, cleaning up the codebase. (tracker/src/mqtt_client.cpp)tracker/src/main.cpp)payloadparameter as[[maybe_unused]]inhandleDatabaseUpdateMessageto suppress unused parameter warnings and clarify intent. (tracker/src/message_handler.cpp)require_array3fromsize_ttorapidjson::SizeTypefor better compatibility with RapidJSON API. (tracker/inc/scene_parser.hpp)✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: