-
Notifications
You must be signed in to change notification settings - Fork 943
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
Various fixes to build MoveIt with clang #1196
Conversation
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.
For what it's worth, this looks like an appropriate fix to me. Thank you @rhaschke.
There is an identical error in planning_scene.h
with the CollisionDetector
struct:
/opt/ros/kinetic/include/moveit/planning_scene/planning_scene.h:967:3: error: 'CollisionDetector' defined as a struct here but previously declared as a class [-Werror,-Wmismatched-tags]
struct CollisionDetector
^
/opt/ros/kinetic/include/moveit/planning_scene/planning_scene.h:964:3: note: did you mean struct here?
class CollisionDetector; typedef std::shared_ptr CollisionDetectorPtr; typedef std::shared_ptr CollisionDetectorConstPtr;;;
^~~~~
struct
2 errors generated.
Thanks for pointing this out. I also added a Travis test to compile with clang. However, this test (using clang 5.0) doesn't complain. There aren't even warnings. |
I am using clang 6.0.0 on Ubuntu 16.04 and ROS Kinetic Kame in the standard PPA binary release and the moveit version that comes with it.
|
Speaking of the PPA binary release, is this fix even likely to make it into there? |
We just recently did a release into Kinetic. So there will not be a new release very soon. |
25fe37a
to
44cb603
Compare
Hm, clang can successfully compile the melodic branch on Melodic/Bionic but not on Kinetic/Xenial. |
That is possible, I didn't check that, sorry. I have the |
Fully agree. Just wanted to confirm that this is an error only. There are dozens of these struct vs. class declaration errors and I don't have cycles to work more on this issue now. |
I have built moveit from source using your PR branch, but still get the error. |
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.
+1 to clang. I'll use this occasion to change my local build to clang too.
Checking the backward compatibility support with clang seems a bit random though.
I would propose to use melodic-melodic-shadow-fixed (latest and greatest) instead.
The clang test fails at the moment though because it can't find the compiler.
You recently added that to the melodic-devel branch directly.
I guess rebasing should resolve this?
add_compile_options(-Wno-maybe-uninitialized) | ||
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | ||
add_compile_options(-Wno-inconsistent-missing-override -Wno-potentially-evaluated-expression) | ||
endif() |
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.
It would be good to have comments in here on why these -Wno-*
flags were added.
Are these warnings something we should improve in our code base?
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.
These flags improve our code style by throwing warnings for common issues. See my PR #971
I'd like to add this to all the packages in moveit
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.
No they don't -Wno-*
actually disables specific warnings. Probably because @rhaschke either decided they are not important for us, or because some package we rely on throws lots of warnings we can't fix ourselves.
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.
Ah right. I think I might have done this because otherwise some of moveit's dependencies (TF I believe) cause warnings that are hard to fix because they are from some of TF's dependencies outside of ROS.
e3120af
to
553c799
Compare
@@ -16,9 +16,13 @@ env: | |||
- TEST_BLACKLIST=moveit,moveit_ros,moveit_runtime,moveit_ros_perception # mesh_filter_test fails due to broken Mesa OpenGL | |||
matrix: | |||
- ROS_DISTRO=kinetic ROS_REPO=ros TEST=clang-format | |||
- ROS_DISTRO=melodic ROS_REPO=ros UPSTREAM_WORKSPACE=moveit.rosinstall | |||
- ROS_DISTRO=melodic ROS_REPO=ros-shadow-fixed UPSTREAM_WORKSPACE=moveit.rosinstall | |||
- ROS_DISTRO=kinetic ROS_REPO=ros-shadow-fixed UPSTREAM_WORKSPACE=moveit.rosinstall |
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.
can we unify these two matrix:
sections into one? if not, please add comment explaining why they are different.
seems like we could move all the tests to the below section with compiler: gcc
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.
could you please still address this question? im curious if this can be improved
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 cannot be merged. As we want to use a different compiler (clang), we need to use the include/matrix section, which allows to add individual entries to the matrix.
add_compile_options(-Wno-maybe-uninitialized) | ||
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | ||
add_compile_options(-Wno-inconsistent-missing-override -Wno-potentially-evaluated-expression) | ||
endif() |
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.
These flags improve our code style by throwing warnings for common issues. See my PR #971
I'd like to add this to all the packages in moveit
4aa1c17
to
207f992
Compare
- use MOVEIT_STRUCT_FORWARD for structs - fix initialization of dynamic-size array - fix implicit conversion for SortableDisabledCollision -> srdf::Model::DisabledCollision - virtual destructors for classes with virtual methods - use delete[] for memory allocated with new[]
simple assignment does the job
4c8d442
to
5532fbd
Compare
This function is only available in KDL >= 1.4.0 (i.e. in Melodic). For backwards-compatibility with Kinetic, provide compile-time switch.
- use existing moveit::planning_interface::getSharedTF(); - switch based on rviz version (instead of roscpp version)
Ready for merging. |
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.
looks good.
Merging.
#ifdef ROS_KINETIC | ||
std::shared_ptr<tf2_ros::Buffer> tf_buffer = PlanningSceneDisplay::getTF2BufferPtr(); | ||
#ifdef RVIZ_TF1 | ||
std::shared_ptr<tf2_ros::Buffer> tf_buffer = moveit::planning_interface::getSharedTF(); |
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.
nice simplification!
Fixes #1195.