Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -Wconversion flag to protect future developments #1053

Merged

Conversation

gwalck
Copy link
Contributor

@gwalck gwalck commented Jun 13, 2023

When working on joint_limits, I faced an issue with a rclcpp::Duration that I constructed using seconds and nanoseconds, but by mistake used a float 0.005 for the seconds. Since the awaited arguments are ints, the float conversion led to a NULL duration which I did not realize for a too long time.

There was no compilation warning, as -Wall does not include -Wconversion, supposedly to avoid too many warnings.
With that flag the warning looks like this

rolling-ctrl-ws/src/ros2_control/joint_limits/test/test_simple_joint_limiter.cpp:224:29: warning: conversion from ‘double’ to ‘int32_t’ {aka ‘int’} changes value from ‘5.0000000000000001e-3’ to ‘0’ [-Wfloat-conversion]
  224 |     rclcpp::Duration period(0.005, 0);

I tested in ros_control package to add -Wconversion compilation flag which created only 2 warnings, that could be easily fixed with explicit cast.

Hence, for protecting future developments from risky mistakes with conversions, I suggest to add this flag in all packages of the repo. I will provide a similar PR for ros2_controllers that were definitively affected by exactly the same Time-at-zero issue.

bmagyar
bmagyar previously approved these changes Jun 13, 2023
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A welcome addition! Thank you

@codecov-commenter
Copy link

Codecov Report

Merging #1053 (cf6ab33) into master (925f5f3) will decrease coverage by 2.27%.
The diff coverage is 35.10%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1053      +/-   ##
==========================================
- Coverage   34.61%   32.34%   -2.27%     
==========================================
  Files          52       93      +41     
  Lines        2981     9793    +6812     
  Branches     1855     6599    +4744     
==========================================
+ Hits         1032     3168    +2136     
- Misses        310      777     +467     
- Partials     1639     5848    +4209     
Flag Coverage Δ
unittests 32.34% <35.10%> (-2.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
controller_manager/src/controller_manager.cpp 38.04% <ø> (-1.67%) ⬇️
controller_manager/src/ros2_control_node.cpp 0.00% <0.00%> (ø)
..._interface/include/hardware_interface/actuator.hpp 100.00% <ø> (ø)
...ce/include/hardware_interface/async_components.hpp 0.00% <0.00%> (ø)
...re_interface/include/hardware_interface/sensor.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/system.hpp 100.00% <ø> (ø)
hardware_interface/src/resource_manager.cpp 47.32% <ø> (-6.30%) ⬇️
hardware_interface/src/sensor.cpp 50.52% <ø> (ø)
hardware_interface/src/system.cpp 55.45% <ø> (ø)
...rface/test/mock_components/test_generic_system.cpp 9.42% <ø> (ø)
... and 70 more

... and 19 files with indirect coverage changes

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merci

@bmagyar bmagyar merged commit 1b83a5b into ros-controls:master Jun 13, 2023
1 check passed
@christophfroehlich
Copy link
Contributor

@Mergifyio backport iron humble

Copy link
Contributor

mergify bot commented Oct 26, 2024

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 26, 2024
(cherry picked from commit 1b83a5b)

# Conflicts:
#	transmission_interface/test/random_generator_utils.hpp
mergify bot pushed a commit that referenced this pull request Oct 26, 2024
(cherry picked from commit 1b83a5b)

# Conflicts:
#	controller_interface/CMakeLists.txt
#	controller_manager/CMakeLists.txt
#	hardware_interface/CMakeLists.txt
#	joint_limits/CMakeLists.txt
#	transmission_interface/CMakeLists.txt
#	transmission_interface/test/random_generator_utils.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants