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

Fix clang-tidy issues #1706

Merged
merged 7 commits into from
Nov 10, 2022
Merged

Fix clang-tidy issues #1706

merged 7 commits into from
Nov 10, 2022

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Nov 9, 2022

Having the clang-tidy check performance-unnecessary-value-param disabled for a few years, lead to the introduction of several shortcomings in the code, which should be fixed asap.
Applying all automatic fixes from clang-tidy is not a good idea as you can see from second commit, trying to cleanup a few of them. Somebody from the MoveIt2 team should take over and clean up the remaining ones. @henningkayser, can you please coordinate this work?

@henningkayser
Copy link
Member

I think I got most of them. I'm still seeing some "candidate function not viable: no known conversion ..." errors locally, will look into it tomorrow.

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 50.97% // Head: 50.98% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (89c9a43) compared to base (a2aa1a6).
Patch coverage: 47.62% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1706      +/-   ##
==========================================
+ Coverage   50.97%   50.98%   +0.01%     
==========================================
  Files         378      378              
  Lines       31649    31657       +8     
==========================================
+ Hits        16131    16137       +6     
- Misses      15518    15520       +2     
Impacted Files Coverage Δ
...e/include/moveit/kinematics_base/kinematics_base.h 76.32% <ø> (ø)
...nclude/moveit/robot_state/cartesian_interpolator.h 50.00% <0.00%> (ø)
...bot_state/include/moveit/robot_state/robot_state.h 90.14% <ø> (ø)
...it_core/robot_state/src/cartesian_interpolator.cpp 39.14% <ø> (ø)
moveit_core/robot_state/src/robot_state.cpp 47.91% <ø> (ø)
...veit/ompl_interface/model_based_planning_context.h 84.45% <ø> (ø)
...oveit_servo/include/moveit_servo/collision_check.h 100.00% <ø> (ø)
...lude/moveit_servo/parameter_descriptor_builder.hpp 100.00% <ø> (ø)
.../moveit_servo/include/moveit_servo/pose_tracking.h 100.00% <ø> (ø)
...ros/moveit_servo/include/moveit_servo/servo_node.h 100.00% <ø> (ø)
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mergify
Copy link

mergify bot commented Nov 10, 2022

This pull request is in conflict. Could you fix it @rhaschke?

@henningkayser
Copy link
Member

With the last commit, clang-tidy succeeds for me locally

@henningkayser henningkayser marked this pull request as ready for review November 10, 2022 13:22
@henningkayser henningkayser added the backport-humble Mergify label that triggers a PR backport to Humble label Nov 10, 2022
Copy link
Contributor Author

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks for handling this, Hennig. There are a few clang warnings left though (which cannot be resolved automatically).

@henningkayser
Copy link
Member

Thanks for handling this, Hennig. There are a few clang warnings left though (which cannot be resolved automatically).

What warnings are you talking about? CI is happy and locally I don't see related warnings or errors anymore either?

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

I don't see any warnings locally, either.

@rhaschke
Copy link
Contributor Author

CI doesn't use -Werror when building with clang. Thus, CI reports success. But if you open the build_target_workspace section, you will see a few warnings. Most of them are related to gmock, which will be fixed via ament/googletest#21 and ament/ament_cmake#408. However, there a few others as well. Maybe you should use -Werror for the clang build as well (if the gmock issues are resolved upstream).

@rhaschke
Copy link
Contributor Author

I approve as well. The warnings can be tackled later as well. This PR should be merged soon to unblock other PRs (which currently will fail due to clang-tidy issues).

@AndyZe AndyZe merged commit 132ad28 into moveit:main Nov 10, 2022
mergify bot pushed a commit that referenced this pull request Nov 10, 2022
* Blindly apply automatic clang-tidy fixes

* Exemplarily cleanup a few automatic clang-tidy fixes

* Clang-tidy fixups

* Missed const-ref fixups

* Fix unsupported non-const -> const

* More fixes

Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
(cherry picked from commit 132ad28)

# Conflicts:
#	moveit_ros/moveit_servo/include/moveit_servo/utilities.h
#	moveit_ros/moveit_servo/src/utilities.cpp
#	moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/plan_solutions.hpp
#	moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
#	moveit_ros/planning/moveit_cpp/src/planning_component.cpp
@rhaschke rhaschke deleted the fix-clang-tidy branch November 11, 2022 07:25
rhaschke added a commit that referenced this pull request Nov 11, 2022
* Blindly apply automatic clang-tidy fixes

* Cleanup wrong fixes

* Missed const-ref fixups

* Fix unsupported non-const -> const

* More fixes

Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
rhaschke added a commit that referenced this pull request Nov 11, 2022
…kport #1703)

* Re-enable clang-tidy check performance-unnecessary-value-param (#1703)
* Fix clang-tidy issues (#1706)

Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
timonegk added a commit to bit-bots/bio_ik that referenced this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants