Skip to content

Fix clang-tidy issues#1706

Merged
AndyZe merged 7 commits intomoveit:mainfrom
rhaschke:fix-clang-tidy
Nov 10, 2022
Merged

Fix clang-tidy issues#1706
AndyZe merged 7 commits intomoveit:mainfrom
rhaschke:fix-clang-tidy

Conversation

@rhaschke
Copy link
Copy Markdown
Contributor

@rhaschke rhaschke commented Nov 9, 2022 •
edited
Loading

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
Copy Markdown
Member

henningkayser commented Nov 9, 2022

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.

@rhaschke rhaschke mentioned this pull request Nov 9, 2022
Merged
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 10, 2022 •
edited
Loading

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
Copy Markdown

mergify bot commented Nov 10, 2022

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

@henningkayser
Copy link
Copy Markdown
Member

henningkayser commented Nov 10, 2022

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
rhaschke
rhaschke commented Nov 10, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

henningkayser commented Nov 10, 2022

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?

AndyZe
AndyZe approved these changes Nov 10, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

rhaschke commented Nov 10, 2022

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
Copy Markdown
Contributor Author

rhaschke commented Nov 10, 2022

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 mergify bot mentioned this pull request Nov 10, 2022
Closed
mergify bot pushed a commit that referenced this pull request Nov 10, 2022
@rhaschke @mergify
Fix clang-tidy issues (#1706)
382f188 
* 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
@rhaschke @henningkayser
Fix clang-tidy issues (#1706)
91db955 
* 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
@mergify @henningkayser @rhaschke
Re-enable clang-tidy check performance-unnecessary-value-param (bac…
b64e97c 
…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>
@henningkayser henningkayser mentioned this pull request Jan 26, 2023
Closed
timonegk added a commit to bit-bots/bio_ik that referenced this pull request Mar 31, 2023
@timonegk
Adapt searchPositionIK to new API
c2b3ab7 
See moveit/moveit2#1706
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndyZe AndyZe AndyZe approved these changes

@vatanaksoytezer vatanaksoytezer Awaiting requested review from vatanaksoytezer

@JafarAbdi JafarAbdi Awaiting requested review from JafarAbdi

@sjahr sjahr Awaiting requested review from sjahr

Assignees

No one assigned

Labels

backport-humble Mergify label that triggers a PR backport to Humble

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rhaschke @henningkayser @AndyZe