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 (backport #1053) #1815

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Oct 26, 2024

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.


This is an automatic backport of pull request #1053 done by [Mergify](https://mergify.com).

Copy link
Contributor Author

mergify bot commented Oct 26, 2024

Cherry-pick of 1b83a5b has failed:

On branch mergify/bp/humble/pr-1053
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit 1b83a5b.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   hardware_interface/src/component_parser.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   controller_interface/CMakeLists.txt
	both modified:   controller_manager/CMakeLists.txt
	both modified:   hardware_interface/CMakeLists.txt
	both modified:   joint_limits/CMakeLists.txt
	both modified:   transmission_interface/CMakeLists.txt
	both modified:   transmission_interface/test/random_generator_utils.hpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@christophfroehlich christophfroehlich force-pushed the mergify/bp/humble/pr-1053 branch from 93e9987 to fa2cee1 Compare October 26, 2024 18:31
Copy link

codecov bot commented Oct 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.55%. Comparing base (8bf8a6c) to head (fa2cee1).
Report is 1 commits behind head on humble.

Additional details and impacted files
@@            Coverage Diff             @@
##           humble    #1815      +/-   ##
==========================================
- Coverage   62.57%   62.55%   -0.02%     
==========================================
  Files         108      108              
  Lines       12025    12027       +2     
  Branches     8129     8132       +3     
==========================================
- Hits         7525     7524       -1     
- Misses        880      882       +2     
- Partials     3620     3621       +1     
Flag Coverage Δ
unittests 62.55% <100.00%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
hardware_interface/src/component_parser.cpp 91.88% <100.00%> (+0.07%) ⬆️

... and 1 file with indirect coverage changes

@bmagyar bmagyar merged commit 24fcfe7 into humble Oct 26, 2024
10 of 11 checks passed
@bmagyar bmagyar deleted the mergify/bp/humble/pr-1053 branch October 26, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants