-
Notifications
You must be signed in to change notification settings - Fork 531
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
Fix cmake warnings #690
Fix cmake warnings #690
Conversation
Codecov Report
@@ Coverage Diff @@
## main #690 +/- ##
==========================================
- Coverage 54.16% 54.14% -0.02%
==========================================
Files 192 192
Lines 20180 20180
==========================================
- Hits 10929 10924 -5
- Misses 9251 9256 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I suggest renaming the PR to something like "Fix build warnings". I found "cmake" in the title somewhat misleading.
There is also one more clang-tidy
issue.
Possible fix for clang-tidy: henningkayser#11 |
997ca07
to
c6a31e9
Compare
8a26425
to
7d98e09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we enforce the build to be free of warnings?
+1, the only problem with this is it may force us to branch off galactic since some features are deprecated in rolling and the alternatives don't exist in galactic
7d98e09
to
7eae5cb
Compare
We do that for moveit, so I think it's a very good idea to enforce it for moveit2 as well!
I would argue that using deprecated API should still be avoided in the newer distro, possibly using |
This fixes the build warnings discovered here https://github.com/ros-planning/moveit2/runs/3662305871?check_suite_focus=true#step:8:689. Should we enforce the build to be free of warnings?